[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:3448
+          // with alias analysis.
+          if (!ShouldSink && !I->mayReadOrWriteMemory()) {
+            auto *Term = UserParent->getTerminator();
----------------
jdoerfert wrote:
> 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?
You are right, this check should be in `TryToSinkInstruction`. I just moved it there.


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

https://reviews.llvm.org/D80120