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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert 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.
----------------
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.


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:3448
+          // with alias analysis.
+          if (!ShouldSink && !I->mayReadOrWriteMemory()) {
+            auto *Term = UserParent->getTerminator();
----------------
mkazantsev wrote:
> This can actually be reduced to `mayReadMemory`, but writes will be rejected in `TryToSinkInstruction` anyways.
I see. I think we should adjust the comment.

Maybe we should not test the instruction at all and instead extend the TryToSink logic.
TryToSink does the test side-effect test already and it can also deal with instructions reading memory, though only when sink to the successor. If this logic is in there we know what tests to apply and which ones not. We also have a clearer path forward for extensions. Maybe I miss a reason to keep the logic here?


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

https://reviews.llvm.org/D80120