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

Sanjoy Das via llvm-commits llvm-commits at lists.llvm.org
Fri Oct 21 10:26:09 PDT 2016

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();`.


More information about the llvm-commits mailing list