This is an archive of the discontinued LLVM Phabricator instance.

Fix shift legalization and lowering for big constants.
ClosedPublic

Authored by chfast on Jun 26 2015, 8:11 AM.

Details

Summary

If shift amount is a constant value > 64 bit it is handled incorrectly during type legalization and X86 lowering. This patch the type of shift amount argument in function DAGTypeLegalizer::ExpandShiftByConstant from unsigned to APInt.

Diff Detail

Event Timeline

chfast updated this revision to Diff 28560.Jun 26 2015, 8:11 AM
chfast retitled this revision from to Fix shift legalization and lowering for big constants..
chfast updated this object.
chfast edited the test plan for this revision. (Show Details)
chfast added a subscriber: Unknown Object (MLST).
chfast set the repository for this revision to rL LLVM.
sanjoy edited edge metadata.Jul 6 2015, 7:36 PM

I don't see anything wrong with the change, but I don't understand the ins and outs (no pun intended!) of SelectionDAG well enough to give an explicit LGTM.

chfast added a comment.Jul 7 2015, 2:35 AM

Ok, thanks, I will wait some more.

The change is not much about SelectionDAG. I just want to avoid calling getZExtValue() at line 2180 when the value of the APInt cannot fit 64 bits.

chfast added a comment.Jul 8 2015, 3:03 AM

However, I would be very happy if I could commit that before 3.7 release is branched.

RKSimon added a subscriber: RKSimon.Jul 8 2015, 6:36 AM

Hi Paweł - comments below.

lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp
2180

I'm not sure about this - ExpandShiftByConstant already handles shift amounts out of range, setting the result to zero/signbit for logical/arithmetic shifts instead of undef.

I think it better to change ExpandShiftByConstant shift amount to take an APInt type instead of an unsigned and update its handling to compare using APInt.

If you want to make a case for returning undef for constant shift results it should be in a separate patch.

test/CodeGen/X86/legalize-shl-vec.ll
3

It would be better if these tests checked the resultant codegen, rather than just test for a crash.

chfast added a comment.Jul 8 2015, 6:38 AM

Thanks Simon for the comments. I will do as you suggested.

chfast updated this revision to Diff 29265.Jul 8 2015, 8:47 AM
chfast edited edge metadata.

I changed DAGTypeLegalizer::ExpandShiftByConstant to accept APInt as the shift amount.
There is one quirk in that: because there is not operator- for pair (uint64_t, APInt)
I workarounded it with strage looking -Amt + NVTBits.

chfast updated this object.Jul 8 2015, 8:48 AM
chfast added a reviewer: RKSimon.
RKSimon accepted this revision.Jul 8 2015, 1:42 PM
RKSimon edited edge metadata.

LGTM, thanks.

This revision is now accepted and ready to land.Jul 8 2015, 1:42 PM
This revision was automatically updated to reflect the committed changes.