[PATCH] D80120: [InstCombine] Sink pure instructions down to return and unreachable blocks

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev marked an inline comment as done.
mkazantsev added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:3453
+          if (ShouldSink) {
+            assert(DT.dominates(BB, UserParent) && "Domination broken?");
             // Okay, the CFG is simple enough, try to sink this instruction.
----------------
jdoerfert wrote:
> mkazantsev wrote:
> > jdoerfert wrote:
> > > mkazantsev wrote:
> > > > jdoerfert wrote:
> > > > > This is not correct for things that throw, things that synchronize, e.g., warp shuffles on GPUs, (and things that loop forever once we have a way to disable forward propagation guarantee).
> > > > > 
> > > > > Please add a readnone call test to expose the above. You can probably ask `I->isSafeToRemove() && !I->mayReadFromMemory()` to determine if it can be moved. Effectively, we might actually remove it from a path that does not end in the user instruction. If the path ends in the user instruction, the fact that we could remove it does mean the value is not interfering with something else. It also doesn't read memory, so we are good.
> > > > > 
> > > > > We still have to fix `isSafeToRemove` wrt. the syncs and endless loops I mentioned above but that is a separate issue.
> > > > > 
> > > > > ---
> > > > > Nit: `Dominance relation broken?`
> > > > > 
> > > > > This is not correct for things that throw, things that synchronize, e.g., warp shuffles on GPUs, (and things that loop forever once we have a way to disable forward propagation guarantee).
> > > > 
> > > > These things are checked inside `TryToSinkInstruction`. It only sinks non-side-effecting things.
> > > Agreed on the side-effects (which include throwing). The shuffles are a separate mess that is not part of this patch. looping forever is not yet a well defined semantic but UB so we are also good on that front. After reading the comment I was not looking into the TryToSink.
> > Actually I'm not sure about "looping forever", it is UB in C++ but I don't remember if LLVM has any conclusive answer whether it is UB in it or not. Anyways, this problem with infinite looping exists in current code as well.
> The "de-facto" answer for now is we assume forward progress guarantees (in various places). The full story is more complicated but hopefully we'll clean it up with an attribute soon. Anyway, not relevant here ;)
> 
> ---
> 
> You can also do: `Terminator->getNumSuccessors() == 0;`
> You can also do: Terminator->getNumSuccessors() == 0;

I think it's true, but I'd rather keep it as is to make it absolutely obvious what we are doing.


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

https://reviews.llvm.org/D80120