[PATCH] D125220: [InstCombine] (rot X, ?) == 0/-1 --> X == 0/-1

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:3043
 
-  if (auto *II = dyn_cast<IntrinsicInst>(Cmp.getOperand(0)))
-    if (Instruction *I = foldICmpIntrinsicWithConstant(Cmp, II, *C))
-      return I;
+  // There are many transforms where we can allow undef, we can solve it here.
+  if (match(Cmp.getOperand(1), m_APIntAllowUndef(C)))
----------------
This comment is not needed. The comment above the new function is enough.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:3289-3290
+        // (rot X, ?) == 0/-1 --> X == 0/-1
+        // This transform is copied from foldICmpEqIntrinsicWithConstant,
+        // it is safe to re-use undef elts in a vector.
+        if (C.isZero() || C.isAllOnes())
----------------
Delete this comment. We did not copy the transform (because that would be bad design). We moved it. 

The safety of allowing undef is already mentioned in the function-level comment and the function name, so there is no need to repeat it here.


================
Comment at: llvm/test/Transforms/InstCombine/icmp-fsh.ll:62
   %rot = tail call <2 x i5> @llvm.fshl.v2i5(<2 x i5>%x, <2 x i5> %x, <2 x i5> %y)
   %r = icmp ne <2 x i5> %rot, <i5 -1, i5 undef>
   ret <2 x i1> %r
----------------
We should add one more test that has "eq zero with undef", so the transform has better coverage. 

If there is not a test for a non-zero and non-negative-one, then please add that too. Then we know the transform does not happen with something like "<i5 undef, i5 1>".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125220