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

[Sanjoy Das via llvm-commits]

sanjoy added inline comments.

================
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,
----------------
There's a comment on why this is needed before the ` Replacements.emplace_back(CS.getInstruction(), GCResult);` bit.

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

================
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.
----------------
reames wrote:
> 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.
We need to have a discussion about this (I'll send out an email shortly); but yes, we'll have to keep both paths till LLILC is able to generate `"gc-transition"` operand bundles.  I think the changes to LLILC should be fairly minimal (and will probably actually reduce complexity in LLILC), but this is really gated on us bringing operand bundles to an adequate level of stability.

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

================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:1561
@@ +1560,3 @@
+    // considered a live reference.
+    CS.getInstruction()->replaceAllUsesWith(Token);
+    CS.getInstruction()->eraseFromParent();
----------------
reames wrote:
> I believe both schemes could use the deferred update safely for the record.  Less divergent code.
I agree that it would be correct to have both schemes to use `DeferredReplacement`; but I suspect that will be more confusing to read since in one case we'd replace one token with another, whereas in the other case we'd replace a normal return value with `gc_result` (so there will be some divergent code anyway).


http://reviews.llvm.org/D13372