-
Notifications
You must be signed in to change notification settings - Fork 296
Fix CI after LLVM upgrade #1897
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
Conversation
r? @folkertdev rustbot has assigned @folkertdev. Use |
Failures like this on s390x:
The carrying_add here is no longer inlined: stdarch/crates/core_arch/src/s390x/vector.rs Lines 4719 to 4734 in 97bf36d
First guess would be that this is a regression from llvm/llvm-project#132976. But I'd expect that to allow more inlining, not less... |
No, the description in that PR is just incorrect. The implementation that matters here is the BasicTTIImpl one introduced in llvm/llvm-project@e26af09, which implemented a subset check. This one then incorrectly made it a strict equality. |
Posted a revert at llvm/llvm-project#152494. |
cc @uweigand Maybe some of the test coverage we have here should be added as LLVM tests too? |
Thanks for catching this!
As part of @nikic 's PR there is now already a test that inlining does happen in this case. Not sure if anything else is necessary. |
So that's fixed ... but we still get a failure on s390x:
From: stdarch/crates/core_arch/src/s390x/vector.rs Lines 4691 to 4698 in 97bf36d
|
I think this one might be due to rust-lang/rust#145144 rather than the LLVM upgrade: https://rust.godbolt.org/z/W9fTGhq1G |
The problem is the SystemZ shouldFormOverflowOp() hook: https://github.com/llvm/llvm-project/blob/04aebbfbe2b40ee947a00c50a6e1ab62405a6c80/llvm/lib/Target/SystemZ/SystemZISelLowering.h#L522-L527 It only allows forming uaddo for i32 and i64, but not i128. The SystemZ backend also has a DAGCombine to recognize (a + b) < a independent of that, but not for InstCombine's canonical a > ~b pattern. |
78d9ceb
to
37ecc87
Compare
I've put up llvm/llvm-project#153557 to fix this in the SystemZ backend. In the meantime, I have switched stdarch to use the intrinsics instead. But now we're hitting a failure on i686-unknown_linux-gnu:
Edit: Reduced: https://rust.godbolt.org/z/5nT35vfM9 |
Upstream issue: llvm/llvm-project#153570 |
Something comical about hitting AVX512 issues on an architecture from 1995. Not that it shouldn't work, of course |
Do we really need to wait for the LLVM fix btw? Seems like we are stepping in (almost) UB territory, because |
@sayantn That's a good point. I've updated the relevant tests to add the This unlocks a new selection failure:
|
Reduced:
It works with Upstream issue: llvm/llvm-project#154492 |
The immediate here encodes both the rounding mode (in the low bits) and the scale (in the high bits). Make sure the scale is non-zero.
Okay, the CI should pass now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sayantn can you confirm that the avx512 changes look good?
#[link_name = "llvm.s390.vaccq"] fn vaccq(a: u128, b: u128) -> u128; | ||
#[link_name = "llvm.s390.vacccq"] fn vacccq(a: u128, b: u128, c: u128) -> u128; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a fix was merged for this s390x issue. Is that just not on the version of LLVM that rustc uses yet?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fix is currently in LLVM 22 only. I wasn't planning to backport it, as it's just an optimization improvement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, it kind of looks like a regression from our perspective given that it worked before, but I can see how from LLVM's perspective this is sort of OK.
This fixes a number of issues after the LLVM upgrade: