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

[Philip Reames via llvm-commits]

reames added a comment.

There's enough changing in this diff that I can't really get a sense for what's going on.  I'm going to need you to split this up a bit.  There appear to be a couple of refactorings hidden within this patch.  Please separate them and submit them.


================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:168
@@ -161,2 +167,3 @@
 typedef DenseSet<Value *> StatepointLiveSetTy;
-typedef DenseMap<Instruction *, Value *> RematerializedValueMapTy;
+typedef DenseMap<AssertingVH<Instruction>, AssertingVH<Value>>
+  RematerializedValueMapTy;
----------------
Changing to an AssertVH is a refactoring, please separate.

================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:193
@@ -185,1 +192,3 @@
 
+static bool CallsGCLeafFunction(ImmutableCallSite CS) {
+  if (isa<IntrinsicInst>(CS.getInstruction()))
----------------
This is the same as isGCLeafFunction in PlaceSafepoints.  Pull into a common utility file.  /Utils/Local.h?

================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:212
@@ +211,3 @@
+
+  for (unsigned i = 0, e = CS.getNumOperandBundles(); i != e; ++i) {
+    OperandBundleUse Bundle = CS.getOperandBundle(i);
----------------
This feels like a missing function on CallSite.  getOperandBundle(Name)?

================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:1370
@@ +1369,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 bit really confused me.  Refactoring?  Or something subtle that's not documented/explained?

================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:1446
@@ +1445,3 @@
+    CallTarget = CS.getCalledValue();
+  } else {
+    // This branch will be gone soon, and we will soon only support the
----------------
This section is a refactoring, please separate.


http://reviews.llvm.org/D13372