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

[Carl Ritson via Phabricator via llvm-commits]

critson added a comment.

Thanks!
I won't submit this for at least 72 hours.



================
Comment at: llvm/lib/CodeGen/MachineSink.cpp:1324
+      } else {
+        if (PI->readsRegister(Reg, TRI))
+          return true;
----------------
ruiling wrote:
> 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.
Yes, I guess this edge case exists.
We need to be careful not to consider dead defs, as AMDGPU block prologue instructions often define $scc as a side-effect. 


================
Comment at: llvm/lib/CodeGen/MachineSink.cpp:1847
 
+    LLVM_DEBUG(dbgs() << "Sink instr " << MI << "\tinto block " << *SuccBB);
+
----------------
ruiling wrote:
> May be better to move this log after the interference check?
I don't think that makes sense, then the debug output will not indicate what was rejected due to interference.  The pre-RA sink outputs this information before performing its checks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121277