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

[Denis Antrushin via Phabricator via llvm-commits]

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


================
Comment at: llvm/lib/CodeGen/FixupStatepointCallerSaved.cpp:53
+
+static cl::opt<unsigned> MaxStatepointsWithRegs(
+    "fixup-max-csr-statepoints", cl::Hidden, cl::init(0),
----------------
skatkov wrote:
> Add a comment that it is for debugging purpose. Please mention that not specifying this option means no restrictions at all.
Will do.


================
Comment at: llvm/lib/CodeGen/FixupStatepointCallerSaved.cpp:127
+  unsigned NumDeoptIdx = VarIdx + 5;
+  unsigned NumDeoptArgs = MI.getOperand(NumDeoptIdx).getImm();
+  MachineInstr::const_mop_iterator MOI(MI.operands_begin() + NumDeoptIdx + 1),
----------------
skatkov wrote:
> SO.getNumDeoptArgsIdx() ?
Yep.


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


================
Comment at: llvm/lib/CodeGen/FixupStatepointCallerSaved.cpp:328
+
+      // Perform trivial copy propagation
+      MachineBasicBlock *MBB = MI.getParent();
----------------
skatkov wrote:
> in a separate function?
Ok


================
Comment at: llvm/lib/CodeGen/FixupStatepointCallerSaved.cpp:529
+      for (auto const &MO : I->operands()) {
+        if (!MO.isReg() || !MO.isDef())
+          return nullptr;
----------------
skatkov wrote:
> If I understand correctly you are processing starting operands and they are defs.
> so the condition MO.isDef() is actually exit condition of the loop (we processed all defs).
> What else we can see as def? In other words what MO.isReg condition means?
`MO.isDef()` asserts that `MO.isReg()`, so one always must check later before checking former


================
Comment at: llvm/lib/CodeGen/FixupStatepointCallerSaved.cpp:618
+    ++NumStatepoints;
+    if (MaxStatepointsWithRegs.getNumOccurrences() &&
+        NumStatepoints >= MaxStatepointsWithRegs)
----------------
skatkov wrote:
> don't you want to rephrase it as
> 
>   unsigned LastStatepointwithRegsIdx = MaxStatepointsWithRegs.getNumOccurrences() ? MaxStatepointsWithRegs : Statepoints.size();
> 
>   for (MachineInstr *I : Statepoints)
>     Changed |= SPP.process(*I, ++NumStatepoints <= LastStatepointwithRegsIdx);
For upstream, maybe.
But since I have downstream changes here, I'd like to keep it that way if you do not mind.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81647