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

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:4556-4557
+    if (match(Op0, m_Shl(m_Specific(Op1), m_One())))
+      return new ICmpInst(ICmpInst::getSignedPredicate(Pred), Op1,
+                          Constant::getNullValue(Op1->getType()));
+    else if (match(Op1, m_Shl(m_Specific(Op0), m_One())))
----------------
Nice! I couldn't tell if there was a simple way to express the transform for all predicates, but that looks right.
To verify, please add at least a couple of Alive2 proof links to the patch summary.


================
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(
----------------
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. 


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

https://reviews.llvm.org/D132888