[PATCH] D25200: [InstCombine] New opportunities for FoldAndOfICmp and FoldXorOfICmp

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

Thanks for adding the code and test comments - this is much easier to follow now. See inline comments for a couple of small items.



================
Comment at: lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:850
+
+  if (RHSCst->isZero() && LHSCst->isZero()) {
+
----------------
amehsan wrote:
> spatel wrote:
> > Can we check that RHSCst == LHSCst and remove the getBitWidth checks below?
> See the respond to a similar comment below.
Sorry for not seeing it, but I don't know how it's possible that:
  (*BGC1.Mask == *BGC2.Mask) && (BGC1.Mask->getBitWidth() != BGC2.Mask->getBitWidth())

Can you add a test case to show where that happens?

To be clear, my proposal is to change the initial check:
  if (RHSCst->isZero() && LHSCst->isZero())
to:
  if (RHSCst->isZero() && LHSCst == RHSCst)

And then remove the bit width check from the later 'if' statement. The operand types of icmp and xor/and must match, so I thought that would be sufficient?


================
Comment at: lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:758-766
+  auto *Inst = dyn_cast<Instruction>(LHS);
+
+  if (!Inst || Inst->getOpcode() != Instruction::And)
+    return BGC;
+
+  // TODO Currently this does not work for vectors.
+  auto *Mask = dyn_cast<ConstantInt>(Inst->getOperand(1));
----------------
Simplify?
  ConstantInt *Mask;
  if (!match(LHS, m_And(m_Value(), m_ConstantInt(Mask))))
    return BGC;



================
Comment at: lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:2817
+      // and we know both A and B are either 8 (power of 2) or 0
+      // we can simplify to (icmp ne A, B)
+      if (Value *Res = FoldXorOfICmps(LHS, RHS))
----------------
Period at end of sentence.


https://reviews.llvm.org/D25200