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

[ChenZheng via Phabricator via llvm-commits]

shchenz added a comment.

The plan sounds good to me. Thanks for doing this @SjoerdMeijer



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


================
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");
----------------
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.


================
Comment at: llvm/lib/CodeGen/MachineSink.cpp:395
+      LLVM_DEBUG(dbgs() << "LoopSink: Instruction not loop invariant\n");
+      continue;
+    }
----------------
This may be unnecessary as all the instructions come from loop preheader?


================
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");
----------------
This seems strange, MI should be a user inside the loop, why its parent block must be equal to the loop preheader?


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

https://reviews.llvm.org/D93694