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

[Ehsan Amiri via Phabricator via llvm-commits]

amehsan added inline comments.


================
Comment at: lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:850
+
+  if (RHSCst->isZero() && LHSCst->isZero()) {
+
----------------
spatel wrote:
> amehsan wrote:
> > 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.  
> *BGC1.Mask == *BGC2.Mask is the APInt operator== and yes, it asserts on bitwidth.
> LHSCst == RHSCst is just an equality check of 2 pointers. Ie, I think there can only be one '0' value of a given bitwidth.
> 
Sorry, I misinterpreted the code. Will change.


https://reviews.llvm.org/D25200