This is an archive of the discontinued LLVM Phabricator instance.

[InstCombine] Remove a couple of asserts based on incorrect assumptions
ClosedPublic

Authored by mgrang on Nov 9 2018, 3:01 PM.

Details

Summary

These asserts are based on the assumption that the order of true/false operands in a select and those in the compare would always be the same.
This fixes PR39595.

Diff Detail

Event Timeline

mgrang created this revision.Nov 9 2018, 3:01 PM

Consider this case:

%9 = icmp ugt i32 %5, %8
%10 = select i1 %9, i32 %8, i32 %5, !prof !2

Here the order of true/false operands of the select and the compare are different. This breaks the checks (X == SI.getTrueValue() && Y == SI.getFalseValue()) and (X == SI.getFalseValue() && Y == SI.getTrueValue())

mgrang edited the summary of this revision. (Show Details)Nov 9 2018, 3:04 PM

Those asserts are validating the metadata correctness. Deleting them is hiding a bug I think.

test/Transforms/InstCombine/select-pr39595.ll
10

I dont' think most of the attributes on this function are necessary to show the bug.

19

Can we hit this bug without these loads?

31

This function_entry_count looks unnecessary to demonstrate the issue.

mgrang updated this revision to Diff 173567.Nov 11 2018, 11:17 AM

Simplified unit test.

mgrang marked 3 inline comments as done.Nov 11 2018, 11:17 AM

@craig.topper Could you please review this? This is a blocker for our internal release.

craig.topper added inline comments.Nov 13 2018, 11:25 AM
Transforms/InstCombine/select-pr39595.ll
9 ↗(On Diff #173567)

I think the branch metadata is incorrect here. It should have been swapped. The original operands were "not %y for true" and "not %x" for false with the false case occuring more often meaning not %x is the most often case. Now the operands are %x for true and %y for false, but the branch metadata is the same so now it says %y occurs more often.

craig.topper added inline comments.Nov 13 2018, 11:26 AM
Transforms/InstCombine/InstCombineSelect.cpp
1823 ↗(On Diff #173567)

I think "Swapped" in the if should be replace with the condition from the assert "X == SI.getFalseValue() && Y == SI.getTrueValue()". Which is what I had in the original patch that went up for review, but was asked to just use Swapped. Which was wrong I guess.

mgrang updated this revision to Diff 173893.Nov 13 2018, 11:42 AM
mgrang set the repository for this revision to rL LLVM.

Addressed comments.

This revision is now accepted and ready to land.Nov 13 2018, 10:48 PM
This revision was automatically updated to reflect the committed changes.