[PATCH] D87154: [Statepoints] Change statepoint machine instr format to better suit VReg lowering.

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

LGTM as well with minor comments.

In abstract, I'd have preferred you split this into the NFC format change and the functionality change for vreg lowering.  I'm not requesting you do so now as the vreg lowering is off by default, so this is effectively NFC for the enabled lowering.



================
Comment at: llvm/lib/CodeGen/SelectionDAG/StatepointLowering.cpp:687
+    Ops.push_back(
+        Builder.DAG.getTargetConstant(GCPtrIndexMap[Base], L, MVT::i64));
+    SDValue Derived = Builder.getValue(SI.Ptrs[i]);
----------------
minor: I'd suggest adding asserts that both Base and Derived are in the map.  They are from above, but local asserts never hurt.  


================
Comment at: llvm/lib/CodeGen/StackMaps.cpp:416
+    for (auto &P : GCPairs) {
+      unsigned BaseIdx = GCPtrIndices[P.first];
+      unsigned DerivedIdx = GCPtrIndices[P.second];
----------------
Again, add an assert index is within bounds.


================
Comment at: llvm/lib/CodeGen/StackMaps.cpp:439
+
 void StackMaps::recordStackMapOpers(const MCSymbol &MILabel,
                                     const MachineInstr &MI, uint64_t ID,
----------------
As a follow up, I might separate this into a parseX routine, and a recordStackMapOpers.  Given Statepoints never set recordResult, there's quite a bit of divergence in semantics here.  This is not a must fix, just an opportunity to clarify the code.  


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87154