[PATCH] D71093: [InstCombine] Insert instructions before adding them to worklist

[Jakub Kuderski via Phabricator via llvm-commits]

kuhar marked 2 inline comments as done.
kuhar added inline comments.


================
Comment at: llvm/include/llvm/Transforms/InstCombine/InstCombineWorklist.h:95-96
+      auto *UI = cast<Instruction>(U);
+      // Do not add instructions that haven't been properly inserted yet.
+      if (UI->getParent()) {
+        Add(UI);
----------------
nikic wrote:
> kuhar wrote:
> > nikic wrote:
> > > kuhar wrote:
> > > > lebedev.ri wrote:
> > > > > The described behavior sounds like a bug to me.
> > > > > More importantly, it's not related to the printing changes?
> > > > The users that I refer to here are new instructions that are being constructed; InstCombine inserts them in subsequent calls to `Add()`. This happens in a few places and it's not obvious to me make it more straight-forward.
> > > > With the changes to the worklist here, everything works like before and the order of iteration is exactly the same.
> > > > 
> > > > Let me know if you want me to dig up some concrete examples for this behavior here.
> > > I'd appreciate an example for this. I can see how it can happen in principle, but not really why we'd write the code that way in practice.
> > Consider the test `llvm/test/Transforms/InstCombine/zext-or-icmp.ll`, function `zext_or_icmp_icmp`:
> > ```
> > define i8 @zext_or_icmp_icmp(i8 %a, i8 %b) {
> >   %mask = and i8 %a, 1
> >   %toBool1 = icmp eq i8 %mask, 0
> >   %toBool2 = icmp eq i8 %b, 0
> >   %bothCond = or i1 %toBool1, %toBool2
> >   %zext = zext i1 %bothCond to i8
> >   ret i8 %zext
> > }
> > ```
> > 
> > Instcombine starts like this:
> > ```
> > INSTCOMBINE ITERATION #1 on zext_or_icmp_icmp
> > IC: ADDING: 6 instrs to worklist
> > IC: Visiting:   %mask = and i8 %a, 1
> > IC: Visiting:   %toBool1 = icmp eq i8 %mask, 0
> > IC: Visiting:   %toBool2 = icmp eq i8 %b, 0
> > IC: Visiting:   %bothCond = or i1 %toBool1, %toBool2
> > IC: Visiting:   %zext = zext i1 %bothCond to i8
> > IC: ADD:   %toBool11 = zext i1 %toBool1 to i8
> > IC: ADD:   %toBool22 = zext i1 %toBool2 to i8
> > IC: ADD:   %1 = xor i8 %mask, 1
> > ```
> > 
> > Instcombine wants to replace all uses of `%toBool11 = zext i1 %toBool1 to i8` with `%1 = xor i8 %mask, 1`. One of the uses is `<badref> = or i8 %toBool11, %toBool22`:
> > ```
> > IC: Replacing   %toBool11 = zext i1 %toBool1 to i8
> >     with   %1 = xor i8 %mask, 1
> > IC: Old =   %zext = zext i1 %bothCond to i8
> >     New =   <badref> = or i8 %1, %toBool22
> > IC: ADD:   %zext = or i8 %1, %toBool22
> > ```
> > 
> > The way I understand the issue is that one of the uses is the instructions that currently being simplified. This instruction will be added to the worklist anyway once the simplification is finished.
> > 
> Thanks for the example. This transform seems to be a pretty special case that manually runs other InstCombine transforms to prevent an infinite loop.
> 
> Assuming that this is the only case (at least I didn't get anything else running InstCombine tests), possibly we might just want to adjust the transform to insert the instruction earlier? Something along these lines:
> 
> ```
> diff --git a/llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp b/llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp
> index 5112fb1a6c3..70a7b9bfbba 100644
> --- a/llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp
> +++ b/llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp
> @@ -1189,7 +1189,8 @@ Instruction *InstCombiner::visitZExt(ZExtInst &CI) {
>        // zext (or icmp, icmp) -> or (zext icmp), (zext icmp)
>        Value *LCast = Builder.CreateZExt(LHS, CI.getType(), LHS->getName());
>        Value *RCast = Builder.CreateZExt(RHS, CI.getType(), RHS->getName());
> -      BinaryOperator *Or = BinaryOperator::Create(Instruction::Or, LCast, RCast);
> +      Value *Or = Builder.CreateOr(LCast, RCast);
> +      Builder.SetInsertPoint(cast<Instruction>(Or));
>  
>        // Perform the elimination.
>        if (auto *LZExt = dyn_cast<ZExtInst>(LCast))
> @@ -1197,7 +1198,7 @@ Instruction *InstCombiner::visitZExt(ZExtInst &CI) {
>        if (auto *RZExt = dyn_cast<ZExtInst>(RCast))
>          transformZExtICmp(RHS, *RZExt);
>  
> -      return Or;
> +      return replaceInstUsesWith(CI, Or);
>      }
>    }
> ```
> 
> I feel like doing this might be better than carving out an exception in the worklist management itself.
You approach seems to like a better way than silently skipping some uses.

I ran the in-source test suite and compiled a few large projects with -O3 to make sure it works.


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

https://reviews.llvm.org/D71093