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

[Philip Reames via llvm-commits]

reames added a comment.

A couple of minor comments for now.  I'm going to return to this Monday.  FYI, the organization of this is really bugging me.  Hopefully, I'll be able to explain why with a bit more thought.  :)


================
Comment at: lib/CodeGen/SelectionDAG/StatepointLowering.cpp:249
@@ +248,3 @@
+static void
+removeDuplicatesGCPtrs(SmallVectorImpl<const Value *> &Bases,
+                       SmallVectorImpl<const Value *> &Ptrs,
----------------
minor: this looks like an unrelated NFCI change.  Please separate and land to simplify review.

================
Comment at: lib/CodeGen/SelectionDAG/StatepointLowering.cpp:345
@@ -346,3 +344,3 @@
   // Export the result value if needed
-  const Instruction *GCResult = ISP.getGCResult();
+  const Instruction *GCResult = SI.GCResult;
   if (HasDef && GCResult) {
----------------
This is going to be confusing named if reused.  For a non-statepoint call with deopt, the GCResult instruction will just be the instruction itself.

================
Comment at: lib/CodeGen/SelectionDAG/StatepointLowering.cpp:375
@@ -377,1 +374,3 @@
+  } else if (SI.IsStatepoint) {
     // The token value is never used from here on, just generate a poison value
+    Builder.setValue(SI.getInstruction(),
----------------
I think this is valid for a non-statepoint void call as well.

================
Comment at: lib/CodeGen/SelectionDAG/StatepointLowering.cpp:392
@@ -393,3 +391,3 @@
     SmallVectorImpl<const Value *> &Bases, SmallVectorImpl<const Value *> &Ptrs,
-    SmallVectorImpl<const Value *> &Relocs, ImmutableStatepoint StatepointSite,
-    SelectionDAGBuilder &Builder) {
+    SmallVectorImpl<const GCRelocateInst *> &Relocs,
+    ImmutableStatepoint StatepointSite, SelectionDAGBuilder &Builder) {
----------------
again, nfci and separate.

================
Comment at: lib/CodeGen/SelectionDAG/StatepointLowering.cpp:580
@@ -592,3 +579,3 @@
   // the alloca
-  for (Value *V : StatepointSite.gc_args()) {
+  for (Value *V : SI.GCArgs) {
     SDValue Incoming = Builder.getValue(V);
----------------
I think the only thing we're using GCArgs for is this loop.  Might be better to just pass in the explicit set of spill slots.


http://reviews.llvm.org/D18106