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

[Denis Antrushin via Phabricator via llvm-commits]

dantrushin marked 5 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:
> 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)




================
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:
> what is the reason for this magic?
The reason is that `TTI.loadRegFromStackSlot` can insert load  only **before** some existing instruction.


================
Comment at: llvm/lib/CodeGen/FixupStatepointCallerSaved.cpp:131
+
+  // Skip deopt args
+  while (NumDeoptArgs--)
----------------
skatkov wrote:
> dantrushin wrote:
> > skatkov wrote:
> > > What if deopt args contains gc pointer?
> > At this point, we can not know.
> > And your code handles all deopt args uniformly already ;)
> > Mine adds some more restrictions 
> This is because I'm sure that GC pointer cannot be on register until your changes.
So nothing has changed.
At this point there is no way to detect deopt pointer which is not in gc list.
ISEL determines what pointers to pass where.
If implementation cannot handle pointer deopt value, not present in gc list, it should not enable it at all or spill **all** deopt values



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81647