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

[Philip Reames via Phabricator via llvm-commits]

reames requested changes to this revision.
reames added a comment.
This revision now requires changes to proceed.

(Comments on code outside StatepointLowering.  Minor +1 question)

Overall, if you address the style comments from the this and the last comment, I think we're close to an LGTM.



================
Comment at: llvm/lib/CodeGen/SelectionDAG/InstrEmitter.cpp:225
   unsigned NumVRegs = HasVRegVariadicDefs ? NumResults : II.getNumDefs();
+  if (Node->getMachineOpcode() == TargetOpcode::STATEPOINT)
+    NumVRegs = NumResults;
----------------
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.)


================
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;
----------------
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.


================
Comment at: llvm/lib/CodeGen/TargetLoweringBase.cpp:1051
       MIB.add(MO);
+      if (TiedTo < i)
+        MIB->tieOperands(TiedTo, MIB->getNumOperands() - 1);
----------------
Please add a comment which explains you're relying on all the definitions coming before any FI and thus the indices not changing.  


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81648