Page MenuHomePhabricator

[InstCombine] Ensure all undef operands are handled before binary instruction constant folding
ClosedPublic

Authored by RKSimon on Mar 20 2016, 1:28 PM.

Details

Summary

As noted in PR18355, this patch makes it clear that all cases with undef operands have been handled before further constant folding is attempted.

Diff Detail

Repository
rL LLVM

Event Timeline

RKSimon updated this revision to Diff 51133.Mar 20 2016, 1:28 PM
RKSimon retitled this revision from to [InstCombine] Ensure all undef operands are handled before binary instruction constant folding.
RKSimon updated this object.
RKSimon added reviewers: majnemer, bogner.
RKSimon set the repository for this revision to rL LLVM.
RKSimon added a subscriber: llvm-commits.
majnemer added inline comments.Mar 20 2016, 2:28 PM
lib/IR/ConstantFold.cpp
1001–1003 ↗(On Diff #51133)

I don't like this because it will silently accept newly added binary operators.
I'd instead prefer if the switch was over the Opcode static_cast'd to the BinaryOps type (perhaps with an assertion that isBinaryOp(Opcode)).

RKSimon updated this revision to Diff 51156.Mar 21 2016, 6:55 AM

Updated so that we assert on unhandled binary instructions with undef operands.

At present we don't handle float binary instructions so I've split these off with a TODO comment instead of asserting - this patch is about clarity not adding the extra handling support.

majnemer added inline comments.Mar 21 2016, 8:36 AM
lib/IR/ConstantFold.cpp
1008–1011 ↗(On Diff #51156)

ConstantFoldBinaryInstruction should only be called with a binary op, I'd move this assert to the start of the function.

RKSimon updated this revision to Diff 51205.Mar 21 2016, 11:58 AM

Moved isBinaryOp assert to start of function - with that done we can remove the 'default' option for undef handling and just assert afterward that neither operand is undef, this will pick up any current/future binary instructions that we don't handle undefs for.

majnemer added inline comments.Mar 21 2016, 12:10 PM
lib/IR/ConstantFold.cpp
923 ↗(On Diff #51205)

Can we make this switch (static_cast<BinaryOps>(Opcode)).

This would make the switch fully-covered.

RKSimon updated this revision to Diff 51227.Mar 21 2016, 2:18 PM

Converted the switch statement to cast to Instruction::BinaryOps - requires that BinaryOpsEnd is added as a case option.

majnemer accepted this revision.Mar 21 2016, 2:24 PM
majnemer edited edge metadata.

LGTM with nits.

lib/IR/ConstantFold.cpp
1010–1011 ↗(On Diff #51227)

I'd make this llvm_unreachable.

This revision is now accepted and ready to land.Mar 21 2016, 2:24 PM
This revision was automatically updated to reflect the committed changes.

Thanks David