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

[Sam Parker via Phabricator via llvm-commits]

samparker added inline comments.


================
Comment at: llvm/lib/CodeGen/MachineSink.cpp:473
+        MachineInstr *I = *It;
+        if (SinkIntoLoop(L, *I)) {
+          EverMadeChange = true;
----------------
SjoerdMeijer wrote:
> 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.
I thought IsSafeToMove is just about side-effects and memory operations? I was thinking purely about the data dependency chain. In your copy example, what's stopping us from skipping the COPY but sinking the ADD? (I understand we'll always sink the COPY really, so let's pretend it's just another general consumer of ADD)


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

https://reviews.llvm.org/D93694