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

[Jakub Kuderski via Phabricator via llvm-commits]

kuhar added a comment.

In D71093#1783809 <https://reviews.llvm.org/D71093#1783809>, @lebedev.ri wrote:

> I'm sorry, i'm not following the patch's logic anymore.
>  First it was about relying on `Add()` printing `ADD:` line, and now
>  all the printing stuff seems to be gone and the patch is adding some
>  invariants on when the instruction should be inserted into worklist?


Since the beginning, the patch was about ensuring that the instructions added to the worklist are properly inserted. One manifestation of instructions not being correctly inserted that was easy to observe was debug `ADD` lines with `<badrefs>` being printed.
One confusing thing that happened was that I followed a suggestion of reducing the amount of debug output printed and removing the line containing `NEW` that directly preceded `ADD`. But this change is no more and the amount of the debug output is the same.

> Does the current diff still achieve the same initial goal of not printing `badref`?

Yes.

> How? If not, the description is not longer applicable, and post a new patch instead?

By ensuring that instructions are inserted. I moved adding instructions later in one place in InstCombine that solves the issue.
In addition, the patch asserts that all inserted instructions have a parent (block). This is stronger than making debug logs pretty, but serves the same goal of ensuring instructions are properly inserted.

> Apologies if i'm horribly misreading the patch?

I think having many iterations made is a bit messy. I also may have simplified the original description of the patch, and though that explaining the change in terms of printing will make it simpler to understand. I apologize and will try to make future patches like this be described more directly.


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

https://reviews.llvm.org/D71093