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

[Sjoerd Meijer via Phabricator via llvm-commits]

SjoerdMeijer added a comment.

Thank you very much for looking at this @shchenz !

In D93694#2504069 <https://reviews.llvm.org/D93694#2504069>, @shchenz wrote:

> Is it possible to implement this new logic to the original MachineSink infra? If so, we can not limit to sink the instruction in loop preheader? I guess we prevent this kind of sinking in machinesink pass is because we treat it as nonprofitable in `MachineSinking::isProfitableToSinkTo`?

It's good you mention this, because this was actually my first approach! :-) I started integrating this logic into the existing code, but run into problems. The main problem was that the existing logic and the new loop sink started interacting, which means I got infinite loops of new blocks being created and things moved. This could probably be fixed, which is what I started doing, but then found that this didn't make things clearer. Thus, since the existing logic and loop sink are slightly different algorithms, I saw the benefit of keeping them separate, for simplicity and clarity. Thus, in this draft, first the original sink logic runs, followed by loop sink. And there might also be a benefit of running them separate and one after the other, as opposed to one integrated solution (that's something I can imagine, but don't have the proof for yet). Long story short, my preference is to keep it separated for now, but that shouldn't exclude reconsidering this if we find a need for this later.

> And we already have some simple register estimation model in `MachineSinking::isProfitableToSinkTo`, should we do this kind of sinking based on register pressure, for example, only sinking to loop if the register pressure of the loop is high?

Exactly right. That is another example and motivation to move this code to here, this is exactly the kind of things I want to reuse. Another example is function `hasStoreBetween`, which is what I am using in D94308 <https://reviews.llvm.org/D94308>. This patch is functionally complete, but I just need to add all the different MIR tests; but again, it's the justification of moving this code to MachineSink. 
Please see also my reply earlier to @samparker, i.e. the four steps that I outlined that are necessary to make loop sinking useful. I am just pointing this out, because using `isProfitableToSinkTo` is exactly one of these next steps to let loop sink make an informed decision. And in other words, this is exactly what I will be doing once we have agreed on this NFC change, this change is the basis for these next steps.



================
Comment at: llvm/lib/CodeGen/MachineSink.cpp:1178
+    // For now assumes that copies are cheap and thus almost always worth it.
+    if (!MI.isCopy()) {
+      CanSink = false;
----------------
shchenz wrote:
> Is this necessary? Assuming MI is a loop user, how does a loop COPY user impact the cost of sinking instruction `I` to the loop? `I` could still be any instruction like a load instruction.
You're absolutely right again, this could be any instruction, and is thus not necessary. But in absence of any cost-modeling or other decision making (based on register pressure), the original authors of this code found that it is always profitable to move the instruction if the user is a copy. 
So yes, this is exactly the first thing that we will be changing when we integrate `MachineSinking::isProfitableToSinkTo` into this work in a next step.


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

https://reviews.llvm.org/D93694