[PATCH] [RewriteStatepointsForGC] Strip deref info after rewriting.

[Philip Reames]

Lots of minor code comments.

As we discussed offline, I'm not particular happy with the idea of this patch.  I can't really see a better solution, but the fact we need to do this for entire module is problematic.  In particular, it breaks the gc per function abstraction...

(Oh, you need to make this conditional on the presence of the GC attribute on at least one function.  Arguably, the changed check handles this, maybe just a clear comment?)


================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:2226
@@ +2225,3 @@
+
+  auto RemoveDerefAttrAtIfNeeded = [&](unsigned Index) {
+    // Code common to handling return values and argument attributes in function
----------------
The IfNeeded doesn't add anything in terms of meaning.

================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:2227
@@ +2226,3 @@
+  auto RemoveDerefAttrAtIfNeeded = [&](unsigned Index) {
+    // Code common to handling return values and argument attributes in function
+    // prototypes.
----------------
Comment should be outside lambda.  I'd be tempted to pull the lambda out as a static helper function since it's fairly large.

================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:2256
@@ +2255,3 @@
+
+  auto RemoveDerefAttrAtIfNeeded = [&](CallSite CS, unsigned Index) {
+    // Code common to handling return values and argument attributes in
----------------
Code duplication ahoy.  Could/should this be commoned with a templated helper function?

================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:2275
@@ +2274,3 @@
+      assert(MD->getNumOperands() < 5 && "unrecognized metadata shape!");
+      bool IsImmutableTBAA =
+          MD->getNumOperands() == 4 &&
----------------
Is there a TBAA helper class we can use here?  Not sure there is, but please check.

================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:2277
@@ +2276,3 @@
+          MD->getNumOperands() == 4 &&
+          mdconst::extract<ConstantInt>(MD->getOperand(3))->getValue() == 1;
+
----------------
It looks like you're only handling one of two forms of TBAA metadata here...

================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:2297
@@ +2296,3 @@
+      if (isa<PointerType>(CS.getType()))
+        RemoveDerefAttrAtIfNeeded(CS, 0);
+    }
----------------
Use Atributes::ReturnIndex both here and above.  No more magic constants!

================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:2375
@@ -2267,2 +2374,3 @@
   MadeChange |= insertParsePoints(F, DT, this, ParsePointNeeded);
+
   return MadeChange;
----------------
Whitespace?

http://reviews.llvm.org/D10105

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/