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

[Denis Antrushin via Phabricator via llvm-commits]

dantrushin marked 2 inline comments as done.
dantrushin added inline comments.


================
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);
----------------
skatkov wrote:
> 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.
GC pointers always occurs in pairs (base, derived).
Only derived pointers are relocated (and so can be tied to defs of statepoint).
If base pointer need to be relocated it will appear as (base, base) pair.

It is not specified if the base pointer must be relocated together with its derived pointer.
At least, this is how I interpret LLVM docs. So I originally wrote it in a way I had no doubts of.

What's the point of having two implementation and comparing them with assert?
Assert is not proof that 'doubtful' implementation is correct. 


================
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.
----------------
skatkov wrote:
> 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.
I will have to implement that function in all backends and then spent 6 months or so asking people
to review change which they won't ever use...
I would like to avoid that


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81647