[PATCH] D147060: [MachineSink] Fix an assertion failure in isCycleInvariant

[antoine moynault via Phabricator via llvm-commits]

antmo added inline comments.


================
Comment at: llvm/lib/CodeGen/MachineCycleAnalysis.cpp:101-103
+  // DebugInstr cannot be moved, mark them as not invariant
+  if (I.isDebugInstr())
+    return false;
----------------
sameerds wrote:
> I am trying to understand how this is relevant to cycle invariance. It seems the intention is that debug instructions cannot be sunk by MachineSink. Then why not do this check there, in the function MachineSinking::FindCycleSinkCandidates()? The check can be done near the same line where isCycleInvariant() is called, ane it will be less confusing. 
Thanks for the review and comment, sameerds!

This first solution was chosen after reading the comment on line 110 in isCycleInvariant (instr can't e.g. be hoisted, so mark this as not invariant). This also made isCycleInvariant callable on more instructions.

But you're right, it is not clear. I will fix this.
It can also be done by checking isSafeToMove before calling isCycleInvariant in FindCycleSinkCandidates.
This will then stay similar to IsLoopInvariant, called only on IsLICMCandidate (safeToMove) instructions.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147060