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

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

Detailed comments on the new implementation.  These are on the whole minor.  Remember to add your new test file.

I need to look more closely at the code outside StatepointLowering, will do that in a separate comment shortly.



================
Comment at: llvm/lib/CodeGen/SelectionDAG/StatepointLowering.cpp:560
+    if (willLowerDirectly(PtrSD) || P->getType()->isVectorTy() ||
+        Builder.DL->isNonIntegralPointerType(P->getType())) {
+      LLVM_DEBUG(dbgs() << "spill "; PtrSD.dump(&Builder.DAG));
----------------
I don't think the isNonIntegralPointer check is needed here.  Can you either remove or explain?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/StatepointLowering.cpp:602
+      continue;
+    }
+    Ptr2ResNo[SDV] = -1;
----------------
This map population isn't related to stack reservation.  Please either separate it into it's own loop, or merge it with the population of the LowerAsVReg set above.

(If I understood this, it's simply the index into a flattened list of the values to be spilled in registers.)


================
Comment at: llvm/lib/CodeGen/SelectionDAG/StatepointLowering.cpp:603
+    }
+    Ptr2ResNo[SDV] = -1;
     reservePreviousStackSlotForValue(SI.Bases[i], Builder);
----------------
I'd suggest simply not adding anything for the map for the spill case.  Using contains checks are more idiomatic of the purpose.


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


================
Comment at: llvm/lib/CodeGen/SelectionDAG/StatepointLowering.cpp:685
 
-    if (Loc.getNode()) {
+    if (Ptr2ResNo[SDV] >= 0) {
+      SpillMap[V] = None;
----------------
See comment above about count checks.  (Contains)


================
Comment at: llvm/lib/CodeGen/SelectionDAG/StatepointLowering.cpp:735
+
+  unsigned NumVRegs =
+      llvm::count_if(Ptr2ResNo, [](auto V) { return V.second >= 0; });
----------------
Isn't this simply Ptr2ResNo.size()?  (after the change suggest above)


================
Comment at: llvm/lib/CodeGen/SelectionDAG/StatepointLowering.cpp:854
+    SDValue SD = getValue(Ptr);
+    if (Ptr2ResNo[SD] >= 0) {
+      NodeTys.push_back(SD.getValueType());
----------------
Again, contains check, not default value.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/StatepointLowering.cpp:1122
 
+  auto &DPtrMap = StatepointLowering.DerivedPtrMap;
+  auto It = DPtrMap.find(DerivedPtr);
----------------
Add a comment describing what this does.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81648