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

[Ehsan Amiri via Phabricator via llvm-commits]

amehsan marked an inline comment as not done.
amehsan added inline comments.


================
Comment at: lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:850
+
+  if (RHSCst->isZero() && LHSCst->isZero()) {
+
----------------
spatel wrote:
> 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?
operator== that is called with (*BGC1.Mask == *BGC2.Mask) has an assertion that the two masks has the same bitwidth. IIUC, if we want to do LHSCst == RHSCst the same operator== is called, and so we would need to check the bit widths of RHSCst and LHSCst. So overall it does not make a difference.  


================
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));
----------------
spatel wrote:
> Simplify?
>   ConstantInt *Mask;
>   if (!match(LHS, m_And(m_Value(), m_ConstantInt(Mask))))
>     return BGC;
> 
Sure.


================
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))
----------------
spatel wrote:
> Period at end of sentence.
Sure.


https://reviews.llvm.org/D25200