[PATCH] D93694: [MachineLICM][MachineSink] Move SinkIntoLoop from MachineLICM to MachineSink

[Sjoerd Meijer via Phabricator via llvm-commits]

SjoerdMeijer added inline comments.


================
Comment at: llvm/lib/CodeGen/MachineSink.cpp:377
+  for (auto &MI : *BB) {
+    LLVM_DEBUG(dbgs() << "LoopSink: Analysing candidate: " << MI);
+    bool DontMoveAcrossStore = true;
----------------
shchenz wrote:
> Should we call `TII->shouldSink()` at the very beginning? Some targets may not want to sink some instructions.
Thanks, agreed.


================
Comment at: llvm/lib/CodeGen/MachineSink.cpp:385
+    if (MI.mayLoad() && !mayLoadFromGOTOrConstantPool(MI) &&
+        !IsGuaranteedToExecute(L, BB)) {
+      LLVM_DEBUG(dbgs() << "LoopSink: Load not guaranteed to execute.\n");
----------------
shchenz wrote:
> Maybe I am wrong, but for the sinking, we must sink an instruction to a block that is dominated by the instruction's parent block. So if the destinate block is executed, BB must also be executed. Do we need the check for `IsGuaranteedToExecute`? I think this is not the same as hoisting. For hoisting, we must make sure the load must be executed as it will be hoisted to the loop preheader which will be executed surely.
yeah, good catch. We are sinking from `preheader` to a loop block `BB`, and `preheader` dominates `BB`. I agree that we don't need this check, this is indeed different for hoisting and sinking. I will remove it.




================
Comment at: llvm/lib/CodeGen/MachineSink.cpp:395
+      LLVM_DEBUG(dbgs() << "LoopSink: Instruction not loop invariant\n");
+      continue;
+    }
----------------
shchenz wrote:
> This may be unnecessary as all the instructions come from loop preheader?
I agreed and changed this into an assert, which seemed like to a good precondition to assert.
But this new assert triggered for one of the test cases where the preheader is the entry block, containing COPY instructions of the function arguments which use physical registers. Physical registers are marked as not loop invariant, which is why this assert triggered. So I think I will just keep this for now.


================
Comment at: llvm/lib/CodeGen/MachineSink.cpp:1219
+    LLVM_DEBUG(dbgs() << "LoopSink:   Analysing use: " << MI);
+    if (!L->contains(&MI) && MI.getParent() != Preheader) {
+      LLVM_DEBUG(dbgs() << "LoopSink:   Use not in loop, can't sink.\n");
----------------
shchenz wrote:
> This seems strange, MI should be a user inside the loop, why its parent block must be equal to the loop preheader?
I thought I had a reason, but it doesn't seem to make sense, so will remove that part of the condition.


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

https://reviews.llvm.org/D93694