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

[Serguei Katkov via Phabricator via llvm-commits]

skatkov added inline comments.


================
Comment at: llvm/lib/CodeGen/FixupStatepointCallerSaved.cpp:131
+
+  // Skip deopt args
+  while (NumDeoptArgs--)
----------------
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.


================
Comment at: llvm/lib/CodeGen/FixupStatepointCallerSaved.cpp:55
+// It may be handy for investigating statepoint spilling issues.
+static cl::opt<unsigned> MaxStatepointsWithRegs(
+    "fixup-max-csr-statepoints", cl::Hidden, cl::init(0),
----------------
Still do not see that not specifying this option actually means infinity.
Because it is a bit strange that 
"Max number of statepoints allowed to pass GC Ptrs in registers" and default value 0.
So it makes me thinking that by default the behavior is off.


================
Comment at: llvm/lib/CodeGen/FixupStatepointCallerSaved.cpp:560
+    if (I->getOpcode() == TargetOpcode::STATEPOINT) {
+      bool Found = llvm::any_of(I->defs(), [Reg](MachineOperand &MO) { return MO.getReg() == Reg; });
+      if (Found)
----------------
Please double check, that there is not problems with subregs here.


================
Comment at: llvm/lib/CodeGen/FixupStatepointCallerSaved.cpp:562
+      if (Found)
+        return &*I;
+    } else if (I->modifiesRegister(Reg, TRI)) {
----------------
If you do not find reg in statepoint you continue to search. For example you found a Reg in previous statepoint, what does it mean?

To me it means that live gc pointer passed through next statepoint and is not mentioned in that statepoint. it is a bug.
I believe you should assert this or stop on the first statepoint.

Providing def as non-first statepoint seems to be a bug,


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81647