This is an archive of the discontinued LLVM Phabricator instance.

[LegalizeTypes][Mips][RISCV][SystemZ] Mask shift amounts in ExpandShiftWithUnknownAmountBit.
AbandonedPublic

Authored by craig.topper on Sep 26 2022, 4:58 PM.

Details

Summary

If the expanded shift will become a libcall we should not pass
out of range shift amounts that did not exist in the original code.

If we're not making a libcall, an out of bounds shift amount would
produce poison, but the selects would block it from propagating. I
think. Though we didn't freeze the shift amount so I'm really not sure.

It's kind of ugly to detect that we're going to make a libcall at this
stage. We'd need to repeat code from ExpandIntRes_Shift and change the
interface of TLI.shouldExpandShift. For now I've taken the simple
approach and always added the mask. Many of the changed tests are
i128 on a 32-bit target which likely isn't exposed to C language code
other than _BitInt.

Fixes PR57988.

Diff Detail

Unit TestsFailed

Event Timeline

craig.topper created this revision.Sep 26 2022, 4:58 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 26 2022, 4:58 PM
craig.topper requested review of this revision.Sep 26 2022, 4:58 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 26 2022, 4:58 PM
Amanieu added inline comments.
llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp
2802

Should this be Amt instead of AmtLack in the argument?

craig.topper added inline comments.Sep 26 2022, 5:07 PM
llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp
2802

Yes

Fix shift amount. This reduces the number of tests changed.

craig.topper retitled this revision from [LegalizeTypes][AMDGPU][Mips][RISCV][X86] Mask shift amounts in ExpandShiftWithUnknownAmountBit. to [LegalizeTypes][Mips][RISCV][SystemZ] Mask shift amounts in ExpandShiftWithUnknownAmountBit..Sep 26 2022, 5:13 PM
RKSimon accepted this revision.Sep 27 2022, 2:36 AM

LGTM

This revision is now accepted and ready to land.Sep 27 2022, 2:36 AM

If we're not allowed to use the libcall with out-of-bounds shift amounts at all, don't we need to unconditionally mask before we generate the libcall? In general, we can speculate out-of-bounds shifts at the IR level; it doesn't make sense to special-case out-of-bounds shifts generated in SelectionDAG.

If we're not allowed to use the libcall with out-of-bounds shift amounts at all, don't we need to unconditionally mask before we generate the libcall? In general, we can speculate out-of-bounds shifts at the IR level; it doesn't make sense to special-case out-of-bounds shifts generated in SelectionDAG.

That's a good point. gcc doesn't appear to mask the call either. Maybe we only need to weaken the pre-condition comment on the function on compiler-rt?

craig.topper abandoned this revision.Sep 27 2022, 12:55 PM

I believe both libgcc and compiler-rt are (differently) "well-behaved" in violating their preconditions; the output isn't very useful, but it terminates without excessive computation and doesn't invoke UB.

Note that the case in PR57988 doesn't involve any out-of-range shifts in the source program. The current codegen means that with (1i128 << 0) you end up calling __ashldi with a shift amount of -64. If that's something we're happy with then the documentation should be fixed and I can go update Rust's compiler-builtins to wrap instead of panicking on out-of-range shifts in debug builds (which is how this bug was discovered).

sethp added a subscriber: sethp.Sep 30 2022, 8:06 AM