[PATCH] D13372: [RS4GC] Use "deopt" operand bundles

[Philip Reames via llvm-commits]

reames added a comment.

Mostly minor comments.  Question about transition args is potentially not minor.


================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:1354
@@ +1353,3 @@
+namespace {
+/// This struct tracks the pending action denoted by the `perform` member
+/// function.  It is used to remember replacements that are not valid to do yet,
----------------
This comment could be far clearer.  I shouldn't have to go look at what perform does to understand the purpose of the class.  

================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:1362
@@ +1361,3 @@
+  explicit DeferredReplacement(Instruction *Old, Instruction *New) :
+    Old(Old), New(New) {}
+
----------------
add assert Old != New.

================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:1364
@@ +1363,3 @@
+
+  void perform() {
+    Instruction *OldI = Old;
----------------
minor: The common name for this would be something like "do".  

================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:1414
@@ +1413,3 @@
+    DeoptArgs = GetDeoptBundleOperands(CS);
+    // We don't fill in TransitionArgs or Flags in this branch, but we could
+    // have an operand bundle for that too.
----------------
Does this mean that to support the MSFT guys, we're going to need both code paths for a while to come?  I think it does.

================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:1530
@@ -1452,3 +1529,3 @@
 
-// The GCResult is already inserted, we just need to find it
 #ifndef NDEBUG
+  if (!UseDeoptBundles) {
----------------
Sinking these asserts into the else clause would be far more clear.  

================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:1543
@@ +1542,3 @@
+      StringRef Name =
+          CS.getInstruction()->hasName() ? CS.getInstruction()->getName() : "";
+      CallInst *GCResult = Builder.CreateGCResult(Token, CS.getType(), Name);
----------------
We've already taken the name for the token above.  This needs adjusted for the new location.

================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:1555
@@ +1554,3 @@
+    } else {
+      Replacements.emplace_back(CS.getInstruction(), nullptr);
+    }
----------------
Wait, what?  Oh, your replacements also does deletion.  Got it.  Slightly confusing.  

================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:1561
@@ +1560,3 @@
+    // considered a live reference.
+    CS.getInstruction()->replaceAllUsesWith(Token);
+    CS.getInstruction()->eraseFromParent();
----------------
I believe both schemes could use the deferred update safely for the record.  Less divergent code.

================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:2415
@@ -2311,1 +2414,3 @@
 
+  std::vector<DeferredReplacement> Replacements;
+
----------------
You need a comment here about what this is and why it's needed.

================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:2429
@@ +2428,3 @@
+  for (auto &PR : Replacements)
+    PR.perform();
+
----------------
Followed by: clear replacements

================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:2436
@@ +2435,3 @@
+
+    // These live sets may contain state Value pointers, since we replaced calls
+    // with operand bundles with calls wrapped in gc.statepoint, and some of
----------------
Any reason this can't be done immediate after inserting the statepoint?  Having it in the next loop after the replacements makes me think there's a reason for that?

================
Comment at: test/Transforms/RewriteStatepointsForGC/deopt-bundles/base-pointers-1.ll:1
@@ +1,2 @@
+; RUN: opt %s -rewrite-statepoints-for-gc -rs4gc-use-deopt-bundles -spp-print-base-pointers -S 2>&1 | FileCheck %s
+
----------------
I am really buged by the fact that you need to duplicate all the test cases like this.  I don't have a good alternative other than what we've already discussed, but I'm worried this is a sign of poor factoring in the code.  


http://reviews.llvm.org/D13372