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

[Serguei Katkov via Phabricator via llvm-commits]

skatkov added a comment.

I need an answer to the question about test and at least update doc for rewriteStatepoint.



================
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:
> > 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. 
It will help you to catch easily an error in case I was wrong with defs.


================
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:
> > 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
ok


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81647