[PATCH] D81647: MIR Statepoint refactoring. Part 3: Spill GC Ptr regs.

[Serguei Katkov via Phabricator via llvm-commits]

skatkov added a comment.

A pretty close to be done.

Do we need a special tests for this patch?



================
Comment at: llvm/lib/CodeGen/FixupStatepointCallerSaved.cpp:125
+// Return statepoint GC args as a set
+static SmallSet<Register, 8> collectGCRegs(MachineInstr &MI) {
+  StatepointOpers SO(&MI);
----------------
dantrushin wrote:
> skatkov wrote:
> > Do I understand correctly that with your changes ALL GC pointers must be defs?
> > So why do you need these iterations instead of just taking all defs?
> Strictly speaking, no. Only derived pointers passed in registers.
> Are we guaranteed that all base pointers will appear as derived ones too?
> If yes, then it is good catch, taking them from defs is simpler (but taking them from operand list instead of def list sounds a bit more natural, IMHO)
> 
> 
I'm a bit confused here. What is the difference between derived and based pointer here?
You have an alive gc pointer. It might be relocated == can be changed. So it must be defined as def independent on whether it is a derived one or based one. Do I miss anything here?

If you are doubt and write that it works under assumption I would suggest under debug assert that sets collected by different way are the same.


================
Comment at: llvm/lib/CodeGen/FixupStatepointCallerSaved.cpp:401
+
+    // To insert reload at the end of MBB, insert it before last instruction
+    // and then swap them.
----------------
dantrushin wrote:
> skatkov wrote:
> > what is the reason for this magic?
> The reason is that `TTI.loadRegFromStackSlot` can insert load  only **before** some existing instruction.
Does it make sense to add an utility function to TTI which after some existing instruction? It looks more natural then this magic.


================
Comment at: llvm/lib/CodeGen/FixupStatepointCallerSaved.cpp:506
+
+    for (Register Reg : RegsToReload)
+      insertReloads(Reg);
----------------
dantrushin wrote:
> skatkov wrote:
> > Don't you want to separate reload loads into separate function?
> > So we'll have:
> > spill registers
> > rewrite statepoint
> > insertReloads/unspill registers
> `insertReloads` uses local vector `RegsToReload` and `MI` (statepoint instruction).
> To call `insertReloads` outside of `rewriteStatepoint` I will have to make that local vector and new statepoint instruction
> available to `insertReloads()`.
> 
> I don't think that making `RegsToReload` member variable or something like that:
> 
> 
> ```
> SmallVector<Register, 8> RegsToReload;
> SS.spillRegisters();
> MachineInstr *NewStatepoint = SS.rewriteStatepoint(RegsToReload); // out parameter
> SS.insertReloads(RegsToReload, NewStatepoint);
> 
> ```
> will be much cleaner.
> But I can do that if you want.
I do not have strong preference here. But separation seems to me makes sense.
At least rename the function rewriteStatepoint and definitely update the comment before the function due to it does additional things except rewriting statepoints.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81647