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

[Sanjoy Das via llvm-commits]

sanjoy marked an inline comment as done.

================
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.
----------------
reames wrote:
> These two fields hint at a problematic coupling of the interface.  Why can't this simply be a list of arguments?
This is because `SelectionDAGBuilder::lowerCallOperands` takes a `CallSite`, `unsigned ArgIdx`, `unsigned NumArgs`.  Passing in an `ArrayRef<Value *> CallArgs` is definitely cleaner, but we'll have to generalize `lowerCallOperands` before that happens.

Edit: I have an idea -- how about we stop using `lowerCallOperands` and use `lowerInvokable` instead?  Then I can embed `TargetLowering::CallLoweringInfo` into `StatepointInfo` and reduce the conceptual duplication.  In fact, the nesting could ultimately be: `StatepointLoweringInfo` contains `DeoptLoweringInfo` contains `CallLoweringInfo`.  I'll try to code that up to see if it works.

================
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) {
----------------
reames wrote:
> 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.
Yes, I'll do that separately if you don't mind.

================
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);
----------------
reames wrote:
> 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?
It is also used below in `if (Relocate->getParent() != StatepointInstr->getParent())`

================
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())
----------------
reames wrote:
> gc-relocate: With all the other users removed, might this code and the startNewStatepoint call be lifted into the parent?
We still need `GCRelocates` in `lowerStatepointInfoMetaArgs`, as noted earlier.

================
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) ==
----------------
reames wrote:
> Why do we need the IsStatepoint flag?  Can't an empty flags convey the same information?
Good point, I'll remove this.


http://reviews.llvm.org/D18106