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

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:908-910
+        B = Builder.CreateLShr(
+            ConstantInt::get(Ty, APInt::getSignMask(Ty->getScalarSizeInBits())),
+            B2);
----------------
spatel wrote:
> lebedev.ri wrote:
> > huihuiz wrote:
> > > lebedev.ri wrote:
> > > > spatel wrote:
> > > > > lebedev.ri wrote:
> > > > > > 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..
> > > > > Hmm...do we have a test where the temporary instruction is created, but the fold fails?
> > > > > Last time I made that mistake, we infinite looped: the temporary instruction gets created and deleted triggering another round of combining and repeat forever.
> > > > Oh, hmm.
> > > Thank you for point this out! Indeed there are chances of infinite looping
> > > 
> > > see test
> > > 
> > > ```
> > > define i1 @foo(i32 %k, i32 %c1, i32 %c2) {
> > >   %t0 = shl i32 3, %c1
> > >   %t1 = and i32 %t0, %k
> > >   %t2 = icmp eq i32 %t1, 0
> > >   %t3 = shl i32 %k, %c2
> > >   %t4 = icmp sgt i32 %t3, -1
> > >   %or = or i1 %t2, %t4
> > >   ret i1 %or
> > > }
> > > 
> > > ```
> > > 
> > > I am working on a solution now.
> > > 
> > Can we just manually delete the instruction if the fold fails?
> > I'm sure that can be nicely wrapped into RAII wrapper.
> I'm skeptical. I could be wrong, but it's not done anywhere else in instcombine?
> Could we send 'B2' out as a partial result (for example, if it's not null, we matched), and then have the caller create the shift if needed?
If we don't create this instruction, i guess e.g. `isCompareToZeroOrEquivalentForm()`, `isKnownToBeAPowerOfTwo()` will need to be also modified, no?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D64275