Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Miscompilation in _mm512_reduce_add_pd, assumes values not NaN #120720

Closed
orlp opened this issue Feb 6, 2024 · 2 comments · Fixed by #120718
Closed

Miscompilation in _mm512_reduce_add_pd, assumes values not NaN #120720

orlp opened this issue Feb 6, 2024 · 2 comments · Fixed by #120718
Assignees
Labels
A-SIMD Area: SIMD (Single Instruction Multiple Data) C-bug Category: This is a bug. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness P-medium Medium priority PG-portable-simd Project group: Portable SIMD (https://github.com/rust-lang/project-portable-simd) requires-nightly This issue requires a nightly compiler in some way. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue.

Comments

@orlp
Copy link
Contributor

orlp commented Feb 6, 2024

The following example should obviously compile to a panic, as the documentation of _mm512_reduce_add_pd mentions nothing about NaNs not being respected:

#![feature(stdsimd)]
use core::arch::x86_64::*;

#[no_mangle]
unsafe fn test() -> f64 {
    let ret = _mm512_reduce_add_pd(_mm512_set_pd(0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, f64::NAN));
    if ret.is_nan() {
        panic!("hi")
    }
    ret
}

However, when compiled with -C opt-level=3 -C target-cpu=cannonlake on the latest nightly there is no panic generated, despite the output being NaN regardless:

.LCPI0_0:
        .quad   0x7ff8000000000000
test:
        vmovsd  xmm0, qword ptr [rip + .LCPI0_0]
        ret
@orlp orlp added the C-bug Category: This is a bug. label Feb 6, 2024
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Feb 6, 2024
@orlp orlp changed the title Miscompilation in _mm512_reduce_add_pd, assumes values are finite and not nan Miscompilation in _mm512_reduce_add_pd, assumes values not nan Feb 6, 2024
@orlp orlp changed the title Miscompilation in _mm512_reduce_add_pd, assumes values not nan Miscompilation in _mm512_reduce_add_pd, assumes values not NaN Feb 6, 2024
@saethlin saethlin self-assigned this Feb 6, 2024
@saethlin saethlin added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness A-SIMD Area: SIMD (Single Instruction Multiple Data) PG-portable-simd Project group: Portable SIMD (https://github.com/rust-lang/project-portable-simd) and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Feb 6, 2024
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Feb 6, 2024
@apiraino
Copy link
Contributor

apiraino commented Feb 7, 2024

WG-prioritization assigning priority (Zulip discussion).

@rustbot label -I-prioritize +P-medium +T-libs +requires-nightly

@rustbot rustbot added P-medium Medium priority requires-nightly This issue requires a nightly compiler in some way. T-libs Relevant to the library team, which will review and decide on the PR/issue. and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Feb 7, 2024
@quaternic
Copy link

Currently the intrinsic is implemented as simd_reduce_add_unordered, which is lowered to LLVM with all the fast-math flags set. That is fixed in PR #120718 .

However, for these _mm512_reduce_{add,mul,min,max}_* intrinsics, the behaviour should match how they are defined in https://www.intel.com/content/www/us/en/docs/intrinsics-guide/index.html#text=float*reduce*ps&expand=4560&ig_expand=5298,5343,5302,5396,5377,5302,5303

All of them follow the same pattern:

  1. (For the masked variants, substitute masked values with an operation-specific identity value.)
  2. Split in half, [lower,upper] = tmp;
  3. Do the operation elementwise as tmp[i] = OP(lower[i], upper[i]);
  4. Repeat from 1. until only one element remains.

saethlin added a commit to saethlin/rust that referenced this issue Feb 20, 2024
…nethercote

Add "algebraic" fast-math intrinsics, based on fast-math ops that cannot return poison

Setting all of LLVM's fast-math flags makes our fast-math intrinsics very dangerous, because some inputs are UB. This set of flags permits common algebraic transformations, but according to the [LangRef](https://llvm.org/docs/LangRef.html#fastmath), only the flags `nnan` (no nans) and `ninf` (no infs) can produce poison.

And this uses the algebraic float ops to fix rust-lang#120720

cc `@orlp`
@bors bors closed this as completed in bb8b11e Feb 21, 2024
github-actions bot pushed a commit to rust-lang/miri that referenced this issue Feb 22, 2024
Add "algebraic" fast-math intrinsics, based on fast-math ops that cannot return poison

Setting all of LLVM's fast-math flags makes our fast-math intrinsics very dangerous, because some inputs are UB. This set of flags permits common algebraic transformations, but according to the [LangRef](https://llvm.org/docs/LangRef.html#fastmath), only the flags `nnan` (no nans) and `ninf` (no infs) can produce poison.

And this uses the algebraic float ops to fix rust-lang/rust#120720

cc `@orlp`
antoyo pushed a commit to rust-lang/rustc_codegen_gcc that referenced this issue Mar 5, 2024
Add "algebraic" fast-math intrinsics, based on fast-math ops that cannot return poison

Setting all of LLVM's fast-math flags makes our fast-math intrinsics very dangerous, because some inputs are UB. This set of flags permits common algebraic transformations, but according to the [LangRef](https://llvm.org/docs/LangRef.html#fastmath), only the flags `nnan` (no nans) and `ninf` (no infs) can produce poison.

And this uses the algebraic float ops to fix rust-lang/rust#120720

cc `@orlp`
bjorn3 pushed a commit to rust-lang/rustc_codegen_cranelift that referenced this issue Mar 8, 2024
Add "algebraic" fast-math intrinsics, based on fast-math ops that cannot return poison

Setting all of LLVM's fast-math flags makes our fast-math intrinsics very dangerous, because some inputs are UB. This set of flags permits common algebraic transformations, but according to the [LangRef](https://llvm.org/docs/LangRef.html#fastmath), only the flags `nnan` (no nans) and `ninf` (no infs) can produce poison.

And this uses the algebraic float ops to fix rust-lang/rust#120720

cc `@orlp`
lnicola pushed a commit to lnicola/rust-analyzer that referenced this issue Apr 7, 2024
Add "algebraic" fast-math intrinsics, based on fast-math ops that cannot return poison

Setting all of LLVM's fast-math flags makes our fast-math intrinsics very dangerous, because some inputs are UB. This set of flags permits common algebraic transformations, but according to the [LangRef](https://llvm.org/docs/LangRef.html#fastmath), only the flags `nnan` (no nans) and `ninf` (no infs) can produce poison.

And this uses the algebraic float ops to fix rust-lang/rust#120720

cc `@orlp`
RalfJung pushed a commit to RalfJung/rust-analyzer that referenced this issue Apr 27, 2024
Add "algebraic" fast-math intrinsics, based on fast-math ops that cannot return poison

Setting all of LLVM's fast-math flags makes our fast-math intrinsics very dangerous, because some inputs are UB. This set of flags permits common algebraic transformations, but according to the [LangRef](https://llvm.org/docs/LangRef.html#fastmath), only the flags `nnan` (no nans) and `ninf` (no infs) can produce poison.

And this uses the algebraic float ops to fix rust-lang/rust#120720

cc `@orlp`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-SIMD Area: SIMD (Single Instruction Multiple Data) C-bug Category: This is a bug. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness P-medium Medium priority PG-portable-simd Project group: Portable SIMD (https://github.com/rust-lang/project-portable-simd) requires-nightly This issue requires a nightly compiler in some way. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants