[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:86
+                           "register spills"),
+                  cl::init(false), cl::Hidden);
+
----------------
samparker wrote:
> Is there a plan to add an option for the maximum number of instructions sunk, or something like that?
Thanks for looking at this!

Getting loop-sinking to do something useful is going to be at least a 3-step approach:

1) This is just moving the code, not changing it, so is a NFC and just a prep step use the infrastructure in MachineSink. Note, that loop-sink is disabled by default, so needs an option to be triggered.
2) A first functional change to make loop-sink a little bit less restrictive, which it really is at the moment, is the change in D94308. This let's it do a bit more analysis using functions in MachineSink, making it a bit more powerful. Nothing changes much: still off by default, but is a good demonstrator for the reshuffle.
3) This is the going to be he interesting step: decision making when and how many instructions to sink. This will be driven by the register pressure, and deciding if reducing live-ranges and loop sinking will help in better performance. I guess this is the place where also a maximum can be introduced and option to control this further, although I agree of course there's in principle nothing stopping us from introducing this earlier.
4) Once we are happy with 3), this should be enabled by default, that should be the end goal of this exercise. 


================
Comment at: llvm/lib/CodeGen/MachineSink.cpp:387
+      continue;
+    if (!MRI->hasOneDef(MO.getReg()))
+      continue;
----------------
samparker wrote:
> What are we doing these checks for?
I think this is being conservative: when an instruction has multiple defs, it's trickier to analyse or we don't want to sink these instructions and skip over it. But I haven't looked much into this (just moved the code), so will need to do that.


================
Comment at: llvm/lib/CodeGen/MachineSink.cpp:1171
+
+  for (MachineInstr &MI : MRI->use_instructions(MO.getReg())) {
+    LLVM_DEBUG(dbgs() << "LoopSink:   Analysing use: "; MI.dump());
----------------
samparker wrote:
> So how do we know that MI is in a loop?
Yeah, good point. I think the only simple answer here is that at this point we don't.
Will look into this.


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

https://reviews.llvm.org/D93694