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

[Tian Zhou via Phabricator via llvm-commits]

tianz marked an inline comment as done.
tianz 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(
----------------
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!


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

https://reviews.llvm.org/D132888