[PATCH] D18106: Extract out a SelectionDAGBuilder::LowerAsStatepoint; NFC

Philip Reames via llvm-commits llvm-commits at lists.llvm.org
Mon Mar 14 15:32:45 PDT 2016


reames added a comment.

The structure of the StatepointInfo struct strongly hints to me that we have a coupling of the API with the implementation.  I understand this is an initial step, but I think it would be good to clean up some of the more messy aspects before moving on.  I've tried to give a couple of specific suggestions below.

In general, I think we should be asking the question: why is StatepointInfo different from CallLoweringInfo?  Merging them might be worth seriously considering.  (I'd suggest NOT doing that in this patch, but keeping that goal in mind and merging them in a later step might be good.)

You might also look closely at the lowerInvokable function in SelectionDAGBuilder.


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.h:739
@@ +738,3 @@
+    /// The set of gc.relocate calls associated with this gc.statepoint.
+    ArrayRef<const GCRelocateInst *> GCRelocates;
+
----------------
The fact we need to pass these in explicitly hints at a broken API.  I'll scatter comments prefixed with gc-relocate: below with suggestions.

================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.h:775
@@ +774,3 @@
+
+    /// The call arguments start at this index in the argument list of the raw
+    /// call site.
----------------
These two fields hint at a problematic coupling of the interface.  Why can't this simply be a list of arguments?

================
Comment at: lib/CodeGen/SelectionDAG/StatepointLowering.cpp:345
@@ -348,3 +344,3 @@
   // Export the result value if needed
-  const Instruction *GCResult = ISP.getGCResult();
+  const Instruction *GCResult = SI.GCResult;
   if (HasDef && GCResult) {
----------------
Could this block of code be lifted into the caller and the GCResult param removed?  It seems unlikely we'll want this for the deopt only use.

================
Comment at: lib/CodeGen/SelectionDAG/StatepointLowering.cpp:528
@@ -536,3 +527,3 @@
   }
-  for (const GCRelocateInst *GCR : Relocations) {
+  for (const GCRelocateInst *GCR : SI.GCRelocates) {
     auto Opt = S.isGCManagedPointer(GCR->getType()->getScalarType());
----------------
gc-relocate: this looks like it should really be an assertion in the visitGCRelocate code.  Separate and commit?

================
Comment at: lib/CodeGen/SelectionDAG/StatepointLowering.cpp:596
@@ -613,2 +595,3 @@
+  for (const GCRelocateInst *Relocate : SI.GCRelocates) {
     const Value *V = Relocate->getDerivedPtr();
     SDValue SDV = Builder.getValue(V);
----------------
gc-relocate: It looks like the only thing we're using the relocates for here is to identify the derived pointer.  Since we've already done that once, can this become a loop over the derived pointers?  Separate and commit?

================
Comment at: lib/CodeGen/SelectionDAG/StatepointLowering.cpp:642
@@ -660,19 +641,3 @@
 #ifndef NDEBUG
-  // Consistency check. Check only relocates in the same basic block as thier
-  // statepoint.
-  for (const User *U : CS->users()) {
-    const CallInst *Call = cast<CallInst>(U);
-    if (isa<GCRelocateInst>(Call) && Call->getParent() == CS.getParent())
-      StatepointLowering.scheduleRelocCall(*Call);
-  }
-#endif
-
-#ifndef NDEBUG
-  // If this is a malformed statepoint, report it early to simplify debugging.
-  // This should catch any IR level mistake that's made when constructing or
-  // transforming statepoints.
-  ISP.verify();
-
-  // Check that the associated GCStrategy expects to encounter statepoints.
-  assert(GFI->getStrategy().useStatepoints() &&
-         "GCStrategy does not expect to encounter statepoints");
+  for (auto *Reloc : SI.GCRelocates)
+    if (Reloc->getParent() == SI.getParent())
----------------
gc-relocate: With all the other users removed, might this code and the startNewStatepoint call be lifted into the parent?

================
Comment at: lib/CodeGen/SelectionDAG/StatepointLowering.cpp:675
@@ -709,2 +674,3 @@
   const bool IsGCTransition =
-      (ISP.getFlags() & (uint64_t)StatepointFlags::GCTransition) ==
+      SI.IsStatepoint &&
+      (SI.StatepointFlags & (uint64_t)StatepointFlags::GCTransition) ==
----------------
Why do we need the IsStatepoint flag?  Can't an empty flags convey the same information?


http://reviews.llvm.org/D18106





More information about the llvm-commits mailing list