[PATCH] D81648: MIR Statepoint refactoring. Part 4: ISEL changes.

[Denis Antrushin via Phabricator via llvm-commits]

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


================
Comment at: llvm/lib/CodeGen/SelectionDAG/InstrEmitter.cpp:225
   unsigned NumVRegs = HasVRegVariadicDefs ? NumResults : II.getNumDefs();
+  if (Node->getMachineOpcode() == TargetOpcode::STATEPOINT)
+    NumVRegs = NumResults;
----------------
reames wrote:
> This line looks unnecessary provided the defs list is marked variadic.  If you want to leave it for the moment, that's fine, but I think we can clean this up in a follow up change.
> 
> (To be specific, OutOperandList in Target.td for STATEPOINT can be variable_ops, and this line disappears.)
`II` here is `MCInstrDesc`.
Statepoint has 0 defs in it MCIntrDesc. (It is `MachineInstr::getNumDefs` which work correctly for variadic outs).
So I really need it here



================
Comment at: llvm/lib/CodeGen/SelectionDAG/ScheduleDAGSDNodes.cpp:128
     const MCInstrDesc &II = TII->get(Def->getMachineOpcode());
-    if (ResNo >= II.getNumDefs() &&
-        II.ImplicitDefs[ResNo - II.getNumDefs()] == Reg)
+    if (ResNo >= II.getNumDefs() && II.hasImplicitDefOfPhysReg(Reg))
       PhysReg = Reg;
----------------
reames wrote:
> I don't understand the implication of this change, can you explain?  On the surface, this looks likely okay, but not understanding the reason for the change is bothering me.
Again, `MCInstr.getNumDefs()` for statepoint always returns 0.
This code assumes that all 'dynamic' defs are implicit, so it accesses II.ImplicitDefs without any checks.
But statepoint has no implicit defs, so for it II.ImplicitDefs is NULL.
That functions does the the same thing, but with proper checks.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/StatepointLowering.cpp:640
   // (base[0], ptr[0], base[1], ptr[1], ...)
   for (unsigned i = 0; i < SI.Bases.size(); ++i) {
+    SDValue Derived = Builder.getValue(SI.Ptrs[i]);
----------------
reames wrote:
> This block of code is functionally broken when base != derived.  You have only added the vreg information for the derived, and would need to spill the base so that the GC can find it.  The fix is trivial, pass "true" for the base case when Base != Derived.
> 
> (Also, this is the probably the profitable lowering, so don't be tempted to add the base to a reg.)
vreg information is only needed for derived pointers to relocate them.
(base pointers are not relocated). So I could just assign all bases to VRegs (if at all possible).
I just did not want to have same value both in stack slot and vreg.

So IMHO the better approach is to check if base is found in `LowerAsVReg` (or that is what you mean?)

As for profitability, our goal is to shift as much spilling as possible to register allocator , doesn't it?




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81648