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

[Jakub Kuderski via Phabricator via llvm-commits]

kuhar marked 3 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:
> > 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.



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

https://reviews.llvm.org/D71093