[PATCH] D132888: [InstCombine] reduce test-for-overflow of shifted value

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: llvm/test/Transforms/InstCombine/icmp-shl-1-overflow.ll:13
+
+define <2 x i1> @icmp_shl_ugt_1_vec(<2 x i8> %x) {
+; CHECK-LABEL: @icmp_shl_ugt_1_vec(
----------------
tianz wrote:
> spatel wrote:
> > 1. We can reduce the tests by 2x without losing much coverage: drop all of the existing vector tests, and change some of the scalar tests to vector types. You can also vary the scalar/element type from i8 to show that the transform doesn't care what the type is (can even use strange types like <3 x i7>). 
> > 
> > 2. For vectors, you can replace a "1" element with "poison" to show that the transform works even if the vector constant is not fully defined (has "don't care" elements).
> > 
> > 3. We also want at least a couple of negative tests to verify that the transform does **not** fire when it is not intended (for example, a test with a signed predicate and a test where the shift is by something other than "1").
> > 
> > 4. Create an extra use of the shl value in at least one test, so we know that the transform works independent of other uses.
> > 
> > 5. If some other transform alters the code before it reaches the new transform, then adjustments will be needed (grep for "thwart complexity-based canonicalization" in this test dir to how to work around operands that get commuted.
> > 
> > Once you have the tests updated, let's pre-commit them with the baseline CHECK lines. That way we can confirm that we are testing the expected patterns. 
> Thanks for all the suggestions! I've updated the tests according to #1 - #4. I don't find "thwart complexity-based canonicalization" necessary, but definitely correct me if I'm wrong.
> 
> For some of the tests, the result code is actually altered after the transform I added, so I had to modify the check to compare against the altered code (e.g. I transform `uge i8 %x, %add` into `sge i8 0, %x`, but the code is then altered by some other transform and eventually becomes `slt i8 %x, 1` ) I'm sure what's the best way to handle that in the tests.
> 
> Regarding your last comment, I'm not exactly sure what you meant. Would you mind giving some examples on how to add the baseline CHECK lines?
> 
> Thanks a lot!
Baseline tests: create a patch with only these tests and the current CHECK lines after running "utils/update_test_checks.py". 
Then, we'll commit those to the main branch to show any transforms we already do (before the code change in this patch).
After that, rebase/update this patch after applying the code change. This patch will then show the test diffs created directly by this patch.

That helps in a few ways: it makes it clear what current output looks like, it makes the review easier by showing diffs, and we won't lose test coverage even if this patch has to be reverted for some reason.

If you don't have commit privileges, you can post the baseline tests as is its own review here on Phabricator, and I can push it for you.

The tests with commuted operands (half of the tests) are not exercising the code path that you want because the operands are changed before we reach the new transform. Example:

```
define <2 x i1> @icmp_shl_ugt_2(<2 x i32> %x) {
  %add = shl <2 x i32> %x, <i32 1, i32 1>
  %cmp = icmp ugt <2 x i32> %x, %add
  ret <2 x i1> %cmp
}

% opt -instcombine icmp.ll -S
...
define <2 x i1> @icmp_shl_ugt_2(<2 x i32> %x) {
  %add = shl <2 x i32> %x, <i32 1, i32 1>
  %cmp = icmp ult <2 x i32> %add, %x  ; these got switched based on operand "complexity"
  ret <2 x i1> %cmp
}

```


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D132888/new/

https://reviews.llvm.org/D132888