[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:
> > 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)
> Yeah, okay. In my examples and added tests, I am sinking only load instructions (probably need to add some more tests). The extra argument `DontMoveAcrossStore` set to true makes this conservative. But you're right that for non-memory ops, this isn't sufficient. In D94308, I added a local `MachineSinking::IsSafeToMove` that adds alias checks.
> 
> So, long story short, this NFC and refactoring is bug-compatible with the original code. I thought separating out this refactoring from any new changes would be best, in order to make this change manageable and not too big. (Please note that this off by default.) This still looks the easiest to me, but if you think this is best, then I will start merging D94308 into this, let me know.
>   
Well how about just adding the break then? I think it's too simple to ignore and would be best to start with a base change that might be correct.


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

https://reviews.llvm.org/D93694