[PATCH] D25096: [RS4GC] New pass to remove gc.relocates added by RS4GC

[Sanjoy Das via llvm-commits]

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

lgtm with comments



================
Comment at: include/llvm/Transforms/Scalar.h:477
+// RewriteStatepointsForGC. The resulting IR is incorrect, but this is useful
+// for analysis
+FunctionPass *createStripGCRelocatesPass();
----------------
End with period.

Also I'd say "useful for manual inspection".  Analysis can also mean "llvm analysis".


================
Comment at: lib/Transforms/Utils/StripGCRelocates.cpp:11
+// This is a little utility pass that removes the gc.relocates inserted by
+// RewriteStatepointsForGC. Note that the generated IR would be incorrect,
+// but this is useful as a single pass in itself, for analysis of IR, without
----------------
s/would be incorrect,/is incorrect,/ sounds better


================
Comment at: lib/Transforms/Utils/StripGCRelocates.cpp:44
+bool StripGCRelocates::runOnFunction(Function &F) {
+
+  // Nothing to do for declarations.
----------------
I'd remove the blank line.


================
Comment at: lib/Transforms/Utils/StripGCRelocates.cpp:57
+  for (GCRelocateInst *GCRel : GCRelocates) {
+
+    // gc.relocates in landing pads are not bound to the statepoint token, these
----------------
Same here: I'd drop the blank line.


================
Comment at: lib/Transforms/Utils/StripGCRelocates.cpp:60
+    // are removed by a following instcombine pass.
+    if (!isStatepoint(GCRel->getOperand(0)))
+      continue;
----------------
Hm -- why is it okay for _instcombine_ to remove the relocates hanging off of landingpads?  In `@test3`, I suspect _instcombine_ is removing these because they aren't used, if they had real uses the landingpad relocates would still be around.

Also, if you avoid adding these landingpad gc relocates to the `GCRelocates` vector then instead of tracking `Changed`, you could just `return !GCRelocates.empty();`.


https://reviews.llvm.org/D25096