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

[Nikita Popov via Phabricator via llvm-commits]

nikic 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);
----------------
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.


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

https://reviews.llvm.org/D71093