[PATCH] D121277: [MachineSink] Check block prologue does not clobber uses

[Ruiling, Song via Phabricator via llvm-commits]

ruiling added a comment.

Thanks for the change, mostly looks good to me. with only a few minor comments.



================
Comment at: llvm/lib/CodeGen/MachineSink.cpp:1307
+    return false; // no prologue
+  for (MachineBasicBlock::iterator PI = BB->begin(); PI != End; ++PI) {
+    // Only check target defined prologue instructions
----------------
maybe start from BB->getFirstNonPHI()? so you don't need to pass PHI to isBasicBlockPrologue().


================
Comment at: llvm/lib/CodeGen/MachineSink.cpp:1324
+      } else {
+        if (PI->readsRegister(Reg, TRI))
+          return true;
----------------
Could you also return true if PI->modifiesRegister()? This is mainly for case like the sunk instruction define whole piece of the register, but there is some prologue instruction overwrite the sub-register. With this check, we would be much like what hasRegisterDependency() has done.


================
Comment at: llvm/lib/CodeGen/MachineSink.cpp:1847
 
+    LLVM_DEBUG(dbgs() << "Sink instr " << MI << "\tinto block " << *SuccBB);
+
----------------
May be better to move this log after the interference check?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121277