-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Annotate function arguments with sext/zext #39051
Conversation
Sounds like an LLVM bug in the PPC backend. It's okay for it to use anyext in the target lowering only because the bits are undef. But the PPC backend codegen is treating them as if they were defined (e.g. missing a trunc & sext before the atomicrmw to legalize the register content) |
I concur with @vtjnash's analysis. |
Doesn't mean that we shouldn't do this also though. Would probably save a few unnecessary instructions in a few places. |
@nemanjai do you have any comments? |
Good to merge then? |
LGTM. |
The PowerPC backend should still be fixed independently. Frankly, I'd prefer for that to happen first before we merge this, since otherwise we risk hiding the bug here. There's nothing preventing a signed comparison with an unsigned type, which would exhibit the exact same symptoms, but with this PR applied the bug is hidden. |
I think we could write a test to show that case too, by passing in a UInt16 and then reinterpreting it as Int16 before the atomic op. But likely easier to show with an llvm unit test. |
Sure, if @vchuravy wants to add that test, I'm happy to merge this ;). |
I will merge this once CI is green. Upstream PR for the PPC backend is https://reviews.llvm.org/D94058 |
backported in #39160 |
https://bugs.llvm.org/show_bug.cgi?id=30451#c11 pointed out that we
need to annotate the parameters of functions with
sext
/zext
so thatthe backend can use the right semantics when promoting from a narrower type
to a register type. We do this for ccall (x-ref: #978) already, but it seems
like we need to do this generally.
Does anybody know if there are other places I might need to add this?