Use -0.0 in `intrinsics::simd::reduce_add_unordered` #130325

workingjubilee · 2024-09-13T20:56:14Z

-0.0 is the actual neutral additive float, not +0.0, and this matters to codegen.

try-job: aarch64-gnu

workingjubilee · 2024-09-13T20:58:28Z

…, r=<try> Use -0.0 in `intrinsics::simd::reduce_add_unordered` -0.0 is the actual neutral additive float, not +0.0, and this matters to codegen. try-job: aarch64-gnu

workingjubilee · 2024-09-13T21:44:26Z

@bors try

…, r=<try> Use -0.0 in `intrinsics::simd::reduce_add_unordered` -0.0 is the actual neutral additive float, not +0.0, and this matters to codegen. try-job: aarch64-gnu

workingjubilee · 2024-09-13T22:45:07Z

@bors try

bors · 2024-09-13T22:46:17Z

⌛ Trying commit 9cb8b36 with merge 5b04398...

…, r=<try> Use -0.0 in `intrinsics::simd::reduce_add_unordered` -0.0 is the actual neutral additive float, not +0.0, and this matters to codegen. try-job: aarch64-gnu

bors · 2024-09-14T00:26:08Z

☀️ Try build successful - checks-actions
Build commit: 5b04398 (5b04398524e45d7f909e42e950354d37d0c29316)

workingjubilee · 2024-09-14T03:18:20Z

@bors try

…, r=<try> Use -0.0 in `intrinsics::simd::reduce_add_unordered` -0.0 is the actual neutral additive float, not +0.0, and this matters to codegen. try-job: aarch64-gnu

…ux, r=RalfJung,jieyouxu Use -0.0 in `intrinsics::simd::reduce_add_unordered` -0.0 is the actual neutral additive float, not +0.0, and this matters to codegen. try-job: aarch64-gnu

…, r=RalfJung,jieyouxu Use -0.0 in `intrinsics::simd::reduce_add_unordered` -0.0 is the actual neutral additive float, not +0.0, and this matters to codegen. try-job: aarch64-gnu

bors · 2024-09-15T07:53:47Z

⌛ Testing commit 914ccea with merge 2301396...

workingjubilee · 2024-09-15T07:56:33Z

@bors retry

…kingjubilee Rollup of 3 pull requests Successful merges: - rust-lang#130295 (Fix target-cpu fpu features on Armv8-R.) - rust-lang#130325 (Use -0.0 in `intrinsics::simd::reduce_add_unordered`) - rust-lang#130371 (Correctly account for niche-optimized tags in rustc_transmute) r? `@ghost` `@rustbot` modify labels: rollup

workingjubilee · 2024-09-15T09:24:05Z

failed in #130387 some kinda assembly parsing edge case

@bors r-

-0.0 is the actual neutral additive float, not +0.0, and this matters to codegen.

workingjubilee · 2024-09-15T23:42:52Z

So Windows now implies SSE3, and so does Apple, so I'm just going to remove the doubled-up x86 tests and just set it to SSE3, because the instructions look almost the same as the AVX set, they just aren't VEX-encoded.

RalfJung · 2024-09-16T05:51:55Z

Makes sense!
@bors r=RalfJung,jieyouxu

bors · 2024-09-16T05:51:58Z

📌 Commit ab8c202 has been approved by RalfJung,jieyouxu

It is now in the queue for this repository.

…kingjubilee Rollup of 5 pull requests Successful merges: - rust-lang#127879 (Document futility of printing temporary pointers) - rust-lang#130325 (Use -0.0 in `intrinsics::simd::reduce_add_unordered`) - rust-lang#130336 (simplify `Build::update_existing_submodule`) - rust-lang#130398 (Add system libs for LLVM when cross compiling for Windows) - rust-lang#130420 (Register tool docs for `src/tools/build_helper`) r? `@ghost` `@rustbot` modify labels: rollup

raoulstrackx · 2024-09-16T07:25:17Z

tests/assembly/simd/reduce-fadd-unordered.rs

+    // aarch64-NEXT: faddp
+
+    // CHECK-NOT: {{f?}}add{{p?s*}}
+    // CHECK: ret


In order to mitigate LVI vulnerabilities, ret instructions are rewritten as popq %rax; lfence; jmpq *rax on the x86_64-fortanix-unknown-sgx target. So this test currently fails on this platform. Can this test be ignored for the SGX target, please?

This sort of thing is a constant issue with the SGX target. Please PR compiletest with an appropriate modification that handles this issue globally without having to modify each and every single test with "oh yeah, and SGX is special, as usual".

I realize I have probably indulged you significantly in the past and I wish to be clear, I do appreciate that Fortanix actually runs the tests in their CI, unlike some, but I must at this point refer to the target tier policy:

Tier 2 targets must not impose burden on the authors of pull requests, or other developers in the community, to ensure that tests pass for the target. In particular, do not post comments (automated or manual) on a PR that derail or suggest a block on the PR based on tests failing for the target. Do not send automated messages or notifications (via any medium, including via @) to a PR author or others involved with a PR regarding the PR breaking tests on a tier 2 target, unless they have opted into such messages.

Like we really need proper turnkey cross-compile testing support per #130375 or even just an SGX exception built in to compiletest or something, twiddling every single test isn't really sustainable for you or for me.

I don't really understand what the appropriate compiletest modification here would be -- unless you are suggesting to skip all assembly tests on SGX?

Assembly tests in general are very finicky, you're lucky if they merge in less than 3 cycles, for one reason or another.

A flat skip wouldn't work for them, a nonzero number of assembly tests are specifically for SGX-related codegen.

Probably, yeah, and that requirement has to go away, which is why I opened #130375

Did you not dereference the pointer the first time I linked it?

Maybe only-x86_64 should exlude SGX, and we have a separate only-sgx for tests that want to run on SGX?

Having only-x86_64 exclude SGX is odd, as SGX is only present on x86_64 platforms. Hence the platform is x86_64-fortanix-unknown-sgx

#130375 is an interesting approach, but I don't see how this would avoid issues like the SGX special case. There isn't a special flag you need to add for the test to succeed on SGX. It's the test itself that causes issues. For most of the exceptions we currently have for SGX, they're there because of the test using CHECK: ret just to denote that they want to find the end of the function. This usually works pretty well because most assembly languages across platforms have identical instructions.

Rollup merge of rust-lang#130325 - workingjubilee:plus-minus-zero-redux, r=RalfJung,jieyouxu Use -0.0 in `intrinsics::simd::reduce_add_unordered` -0.0 is the actual neutral additive float, not +0.0, and this matters to codegen. try-job: aarch64-gnu

…dered_test, r=RalfJung Ignore reduce-fadd-unordered on SGX platform rust-lang#130325 added the `tests/assembly/simd/reduce-fadd-unordered.rs` test. Unfortunately, the use of `CHECK: ret` makes that this test is not compatible with LVI mitigations applied for the SGX target. This PR makes sure this test is ignored for the SGX target, until a nicer solution is available.

Rollup merge of rust-lang#130436 - fortanix:raoul/fix_reduce_add_unordered_test, r=RalfJung Ignore reduce-fadd-unordered on SGX platform rust-lang#130325 added the `tests/assembly/simd/reduce-fadd-unordered.rs` test. Unfortunately, the use of `CHECK: ret` makes that this test is not compatible with LVI mitigations applied for the SGX target. This PR makes sure this test is ignored for the SGX target, until a nicer solution is available.

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Sep 13, 2024

workingjubilee added O-AArch64 Armv8-A or later processors in AArch64 mode A-simd Area: SIMD (Single Instruction Multiple Data) A-floating-point Area: Floating point numbers and arithmetic A-intrinsics Area: intrinsics labels Sep 13, 2024