This is an archive of the discontinued LLVM Phabricator instance.

[InstCombine] Avoid faulty combines of select-cmp-br
ClosedPublic

Authored by bjope on Feb 28 2017, 5:38 AM.

Details

Summary

When InstCombine is optimizing certain select-cmp-br patterns
it replaces the result of the select in uses outside of the
basic block containing the select. This is only legal if the
path from the select to the outside use is disjoint from all
other paths out from the originating basic block.

The problem found was that InstCombiner::replacedSelectWithOperand
did not consider the case when both edges out from the br pointed
to the same label. In that case the paths aren't disjoint and the
transformation is illegal. This patch avoids the faulty rewrites
by verifying that there is a single flow to the successor where
we want to replace uses.

Event Timeline

bjope created this revision.Feb 28 2017, 5:38 AM
spatel added inline comments.Mar 1 2017, 11:37 AM
test/Transforms/InstCombine/select-cmp-br.ll
248–256

Please run 'opt -instnamer' or manually add some names to the test. That makes it easier to see what is happening.

bjope added inline comments.Mar 1 2017, 12:38 PM
test/Transforms/InstCombine/select-cmp-br.ll
248–256

Ok, I tried to just follow the pattern with anonymous names as used in the other tests in this file. But I can definitely add some names to my new test. I guess, in particular, that it is hard to see the connection between "label %4" and "<label>:2".

Btw, do you have any thoughts about how to verify this?
The purpose with this test is to verify that the select-cmp-br rewrite won't trigger. So the checks that I've added actually verifies some other instcombine rewrites that happens instead of select-cmp-br.
I could have added CHECK-NOT checks instead, to only verify that we do not get the faulty pattern that was generated before my correction in replacedSelectWithOperand. The drawback with such a solution is that there are lots of ways to do a faulty optimization and I can not CHECK-NOT all of them. So I preferred to have CHECKs that validates that the transformed code is correct (to a certain degree, similar to other tests in this file...).

spatel added inline comments.Mar 1 2017, 1:27 PM
test/Transforms/InstCombine/select-cmp-br.ll
248–256

Ah, sorry - I didn't even look at the rest of the file. Please update past rL296673. Hopefully, that takes care of the other tests. :)

I definitely agree that you want to CHECK for correctness rather than CHECK-NOT for bugs. We have a script that makes this easy at utils/update_test_checks.py, so you can use that (that's what I used for the other tests).

I'm not sure if you can write a test for this that doesn't excite any other transforms, but that's ok as long we're checking the complete output to make sure that it is correct.

bjope updated this revision to Diff 90301.Mar 2 2017, 1:52 AM

Updated the test case (as suggested by spatel).

  • Rebased to later revision on trunc
  • Removed the irrelevant add from test6
  • Generated names using opt --instnamer
  • Generated checks using utils/update_test_checks.py
spatel accepted this revision.Mar 2 2017, 6:14 AM

LGTM.

This revision is now accepted and ready to land.Mar 2 2017, 6:14 AM
This revision was automatically updated to reflect the committed changes.