[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:53
+
+static cl::opt<unsigned> MaxStatepointsWithRegs(
+    "fixup-max-csr-statepoints", cl::Hidden, cl::init(0),
----------------
Add a comment that it is for debugging purpose. Please mention that not specifying this option means no restrictions at all.


================
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),
----------------
SO.getNumDeoptArgsIdx() ?


================
Comment at: llvm/lib/CodeGen/FixupStatepointCallerSaved.cpp:131
+
+  // Skip deopt args
+  while (NumDeoptArgs--)
----------------
What if deopt args contains gc pointer?


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


================
Comment at: llvm/lib/CodeGen/FixupStatepointCallerSaved.cpp:529
+      for (auto const &MO : I->operands()) {
+        if (!MO.isReg() || !MO.isDef())
+          return nullptr;
----------------
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?


================
Comment at: llvm/lib/CodeGen/FixupStatepointCallerSaved.cpp:618
+    ++NumStatepoints;
+    if (MaxStatepointsWithRegs.getNumOccurrences() &&
+        NumStatepoints >= MaxStatepointsWithRegs)
----------------
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);


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81647