[PATCH] D64275: [InstCombine] Generalize InstCombiner::foldAndOrOfICmpsOfAndWithPow2().

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added a comment.

I think this looks ok, but i'd like to see more test coverage :)



================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:888
+
+      if (match(Compare->getOperand(0),
+                m_OneUse(m_And(m_Value(A), m_Value(B)))))
----------------
could just `return match(...);`


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:889
+      if (match(Compare->getOperand(0),
+                m_OneUse(m_And(m_Value(A), m_Value(B)))))
+        return true;
----------------
Some test should "regress" (improve due to this), could you please add similar extra-use test coverage for this too?


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:908-910
+        B = Builder.CreateLShr(
+            ConstantInt::get(Ty, APInt::getSignMask(Ty->getScalarSizeInBits())),
+            B2);
----------------
To be pointed out, this indeed creates an instruction while we don't know yet whether we will be able to do the fold,
not great, but i think this is the smallest evil, the alternative solution would indeed be uglier..


================
Comment at: llvm/test/Transforms/InstCombine/onehot_merge.ll:27-32
   %t = shl i32 1, %c1
   %t4 = shl i32 1, %c2
   %t1 = and i32 %t, %k
   %t2 = icmp eq i32 %t1, 0
   %t5 = and i32 %t4, %k
   %t6 = icmp eq i32 %t5, 0
----------------
Similarly, could you please add 6 tests with different of each of these instructions having an extra use?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D64275