Page MenuHomePhabricator

[DAGCombiner] Better support for shifting large value type by constants
ClosedPublic

Authored by RKSimon on Aug 1 2016, 3:12 AM.

Details

Summary

As detailed on D22726, much of the shift combining code assume constant values will fit into a uint64_t value and calls ConstantSDNode::getZExtValue where it probably shouldn't (leading to asserts). Using APInt directly avoids this problem but we encounter other assertions if we attempt to compare/operate on 2 APInt of different bitwidths.

This patch adds a helper function to ensure that 2 APInt values are zero extended as required so that they can be safely used together. I've only added an initial example use for this to the '(SHIFT (SHIFT x, c1), c2) --> (SHIFT x, (ADD c1, c2))' combines. Further cases can easily be added as required.

Diff Detail

Repository
rL LLVM

Event Timeline

RKSimon updated this revision to Diff 66293.Aug 1 2016, 3:12 AM
RKSimon retitled this revision from to [DAGCombiner] Better support for shifting large value type by constants.
RKSimon updated this object.
RKSimon set the repository for this revision to rL LLVM.
RKSimon added a subscriber: llvm-commits.

Isn't the IR already undefined whenever this triggers (unless you're working on an i18446744073709551616)?

Isn't the IR already undefined whenever this triggers (unless you're working on an i18446744073709551616)?

If the offending IR originated during dag combine then this is what should be reducing it to UNDEF. And when we are combining repeated shifts with the outer shift being valid then the inner shift might not have been reduced to UNDEF yet.

Ah, of course. undef isn't unrestrained UB, we still have to compile it on the assumption that it's never dynamically executed.

lib/CodeGen/SelectionDAG/DAGCombiner.cpp
731–739 ↗(On Diff #66293)

Lower-case 'z' for functions.

A simpler body would be:

unsigned Bits = std::max(LHS.getBitWidth(), RHS.getBitWidth());
LHS = LHS.zextOrSelf(Bits);
RHS = RHS.zextOrSelf(Bits);
4484 ↗(On Diff #66293)

I don't think this is quite the right check, though it might not matter since it's UB anyway (e.g. c1 = c2 = 2^(Bits-1)). A comment that we made the decision intentionally might be useful though.

RKSimon updated this revision to Diff 67349.Aug 9 2016, 7:50 AM

Updated based on Tim's feedback. Added better support for shift overflow.

I took the easy way out and didn't add support for i18446744073709551616 ;-)

t.p.northover accepted this revision.Aug 9 2016, 9:12 AM
t.p.northover added a reviewer: t.p.northover.

Thanks Simon! Looks good to me.

This revision is now accepted and ready to land.Aug 9 2016, 9:12 AM
This revision was automatically updated to reflect the committed changes.