Skip to content

[AArch64][GlobalISel] llvm.fptoui.sat.i32.f128 miscompile #127804

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

Closed
nikic opened this issue Feb 19, 2025 · 6 comments
Closed

[AArch64][GlobalISel] llvm.fptoui.sat.i32.f128 miscompile #127804

nikic opened this issue Feb 19, 2025 · 6 comments

Comments

@nikic
Copy link
Contributor

nikic commented Feb 19, 2025

Per rust-lang/compiler-builtins#760, LLVM 20 appears to be miscompiling fptoui.sat.i32.f128 on AArch64 when GlobalISel is used.

Assembly for LLVM 19 and LLVM 20: https://llvm.godbolt.org/z/n3K4MEG7r

Likely related to #96297, cc @davemgreen

@llvmbot
Copy link
Member

llvmbot commented Feb 19, 2025

@llvm/issue-subscribers-backend-aarch64

Author: Nikita Popov (nikic)

Per https://github.com/rust-lang/compiler-builtins/pull/760, LLVM 20 appears to be miscompiling fptoui.sat.i32.f128 on AArch64 when GlobalISel is used.

Assembly for LLVM 19 and LLVM 20: https://llvm.godbolt.org/z/n3K4MEG7r

Likely related to #96297, cc @davemgreen

@nikic
Copy link
Contributor Author

nikic commented Feb 19, 2025

Possibly I'm misreading the code, but I suspect that the select operands are inverted?

auto MaxC = MIRBuilder.buildFConstant(SrcTy, MinFloat);
auto MaxP = MIRBuilder.buildFCmp(CmpInst::FCMP_ULT,
SrcTy.changeElementSize(1), Src, MaxC);
auto Max = MIRBuilder.buildSelect(SrcTy, MaxP, Src, MaxC);
This is doing Src < MinFloat ? Src : MinFloat, but surely it should be Src < MinFloat ? MinFloat : Src?

It doesn't really help that MaxC is referring to the minimum float...

@davemgreen
Copy link
Collaborator

Hi - Yeah it sounds like I translated the code from using fminnum/fmaxnum badly..

Do you want me to put together a patch?

@nikic
Copy link
Contributor Author

nikic commented Feb 19, 2025

Hi - Yeah it sounds like I translated the code from using fminnum/fmaxnum badly..

Do you want me to put together a patch?

Yes, please :)

tgross35 added a commit to tgross35/compiler-builtins that referenced this issue Feb 19, 2025
Pin aarch64-unknown-linux-gnu to nightly-2025-02-07 until [1] is
resolved.

[1]: llvm/llvm-project#127804
tgross35 added a commit to rust-lang/compiler-builtins that referenced this issue Feb 19, 2025
Pin aarch64-unknown-linux-gnu to nightly-2025-02-07 until [1] is
resolved.

[1]: llvm/llvm-project#127804
@tstellar tstellar moved this from Needs Triage to Needs Fix in LLVM Release Status Feb 20, 2025
@davemgreen
Copy link
Collaborator

/cherry-pick 70ed381

@llvmbot llvmbot closed this as completed Feb 20, 2025
@github-project-automation github-project-automation bot moved this from Needs Fix to Done in LLVM Release Status Feb 20, 2025
@llvmbot
Copy link
Member

llvmbot commented Feb 20, 2025

/pull-request #128001

tgross35 added a commit to tgross35/rust-libm that referenced this issue Feb 24, 2025
Pin aarch64-unknown-linux-gnu and aarch64-apple-darwin to
nightly-2025-02-07 until [1] makes it to a Rust nightly.

[1]: llvm/llvm-project#127804
tgross35 added a commit to rust-lang/libm that referenced this issue Feb 24, 2025
Pin aarch64-unknown-linux-gnu and aarch64-apple-darwin to
nightly-2025-02-07 until [1] makes it to a Rust nightly.

[1]: llvm/llvm-project#127804
tgross35 added a commit to rust-lang/libm that referenced this issue Apr 18, 2025
Pin aarch64-unknown-linux-gnu and aarch64-apple-darwin to
nightly-2025-02-07 until [1] makes it to a Rust nightly.

[1]: llvm/llvm-project#127804
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

No branches or pull requests

4 participants