[PATCH] D13491: [RS4GC] Refactoring to make a later change easier, NFCI

[Philip Reames via llvm-commits]

reames accepted this revision.
reames added a comment.
This revision is now accepted and ready to land.

LGTM w/comments addressed.


================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:1354
@@ -1357,6 +1353,3 @@
 
-  // Copy all of the arguments from the original statepoint - this includes the
-  // target, call args, and deopt args
-  SmallVector<llvm::Value *, 64> Args;
-  Args.insert(Args.end(), CS.arg_begin(), CS.arg_end());
-  // TODO: Clear the 'needs rewrite' flag
+  Statepoint SP(CS);
+
----------------
For clarity, I might call this OldSP.

================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:1357
@@ +1356,3 @@
+  ArrayRef<Value *> GCArgs(LiveVariables);
+  uint64_t StatepointID = SP.getID();
+  uint32_t NumPatchBytes = SP.getNumPatchBytes();
----------------
const?

================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:1359
@@ -1363,1 +1358,3 @@
+  uint32_t NumPatchBytes = SP.getNumPatchBytes();
+  StatepointFlags Flags = StatepointFlags(SP.getFlags());
 
----------------
This doesn't look right.  You're converting a bitfield to an emum representing what the bits mean.  I think you want the full bitfield.  

================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:1374
@@ +1373,3 @@
+        StatepointID, NumPatchBytes, CallTarget, Flags, CallArgs,
+        TransitionArgs, DeoptArgs, GCArgs, "safepoint_token");
+
----------------
In a separate change, might be better to use the name of the original statepoint.  That will cause test churn, so please do separately if at all.  

================
Comment at: test/Transforms/RewriteStatepointsForGC/relocation.ll:97
@@ -96,3 +96,3 @@
 ; CHECK-DAG: [ %arg.relocated, %if_branch ]
-; CHECK-DAG: [ %arg.relocated4, %else_branch ]
+; CHECK-DAG: [ %arg.relocated3, %else_branch ]
 ; CHECK-NOT: phi
----------------
I'm not seeing why this changed?  Is this a non-determinism?  Or did something in your patch effect ordering?


http://reviews.llvm.org/D13491