[PATCH] D30455: [InstCombine] Avoid faulty combines of select-cmp-br

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: test/Transforms/InstCombine/select-cmp-br.ll:160-168
+define i32 @test6(i32, i1) {
+entry:
+  %2 = select i1 %1, i32 %0, i32 0
+  %3 = icmp eq i32 %2, 0
+  br i1 %3, label %4, label %4
+
+; <label>:2                                      ; preds = %entry, %entry
----------------
bjope wrote:
> spatel wrote:
> > Please run 'opt -instnamer' or manually add some names to the test. That makes it easier to see what is happening.
> 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...).
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.


https://reviews.llvm.org/D30455