[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:
> 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.


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

https://reviews.llvm.org/D80120