[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:473
+        MachineInstr *I = *It;
+        if (SinkIntoLoop(L, *I)) {
+          EverMadeChange = true;
----------------
samparker wrote:
> Do we need to break here as soon as SinkIntoLoop is false? Otherwise, couldn't we sink an 'earlier' instruction that is an input to an instruction that hasn't been sunk?
Unless I miss something, but I don't think so. I think that is too conservative. For example, if we have a number of sink candidates, and stop after e.g. the very first one if that can't be sunk for some reason, then we miss a lot of opportunities if this instruction is completely Independent from the other candidates.

Correct me if I am wrong, but I think what you describe is simply a case of not "IsSafeToMove": there is a dependency that we must respect and can't break.
That's why I also walk the candidates in reverse order. Because for a little def-use chain in a preheader, these 2 candidate instructions: 

    %12:gpr64sp = nuw ADDXri %9, 12, 0
    %2:gpr64all = COPY %12

if we start with trying to move ADD, then that has a use in the same block, and we can't move the def passed that. If we first move the COPY, then the ADD, or at least try that, we can move the partial/whole chain.


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

https://reviews.llvm.org/D93694