Page MenuHomePhabricator

[RS4GC] New pass to remove gc.relocates added by RS4GC
ClosedPublic

Authored by anna on Sep 30 2016, 8:23 AM.

Details

Summary

Utility pass to remove gc.relocates created by rewrite statepoints for GC.
With respect to safepoint verification, the IR generated would be incorrect, and cannot run on VMs
as such. However, the IR verifier would verify the IR to be correct.

This would be a single transformation on the final optimized IR. LLVM used with most VMs would have the RewriteStatepointsForGC (RS4GC) enabled in the opt pipeline, or some form of garbage collection support. See: http://llvm.org/docs/Statepoints.html

The benefit of this pass is for easy analysis of the IR, when the final optimized IR is 'polluted' by too
many gc.relocates of the original pointers. For example, in manual performance analysis of the optimized IR, we can walk back through the use-def chain without having to deal with all the gc.relocates.

test run: All RS4GC tests with -verify option. Local downstream tests on large
IR files. This also works when the pointer being gc.relocated is another
gc.relocate.

TODO: There are 2 specific parts that need some more thought. Right now most
GC.relocates are removed.

Diff Detail

Repository
rL LLVM

Event Timeline

anna updated this revision to Diff 73056.Sep 30 2016, 8:23 AM
anna retitled this revision from to [RS4GC] New pass to remove gc.relocates added by RS4GC.
anna updated this object.
anna added reviewers: sanjoy, reames.
anna added a subscriber: llvm-commits.
anna added inline comments.Sep 30 2016, 8:30 AM
lib/Transforms/Utils/RemoveGCRelocates.cpp
77–78 ↗(On Diff #73056)

These are cases where lets say original pointer is i32*, while the gc.relocate are of type i8*.

All uses of the gc.relocates are phi or other non-store operations, which use the i8* relocated pointer. In such a case, there is no bitcast from i8* to i32* (since there are no uses of the i32* version of relocated pointer). I think this can be transformed to:

original_ptr.casted = bitcast i32* original_ptr to i8*
replace all uses of relocated pointer with original_ptr.casted
anna added inline comments.Sep 30 2016, 9:07 AM
lib/Transforms/Utils/RemoveGCRelocates.cpp
60–61 ↗(On Diff #73056)

The first line (A gc.relocate is bound to a single statepoint token) is not relevant here.

Comment should be:
The order of removing the gc.relocate does not matter, since we first make sure that *all* uses of the relocate are replaced with original pointer. After completion of this transformation for all gc.relocates, we would have the original pointer in all relocates (including nested gc.relocate).

sanjoy requested changes to this revision.Sep 30 2016, 11:09 AM
sanjoy edited edge metadata.

I have some minor comments inline, but overall I'm not sure if a pass like this (solely useful for generating "debugging output") is okay to go upstream.

Do you mind starting a thread on llvm-dev discussing if this sort of thing is okay with the community?

lib/Transforms/Utils/RemoveGCRelocates.cpp
1 ↗(On Diff #73056)

Fix the comment here.

41 ↗(On Diff #73056)

Not sure if you need private: here.

69 ↗(On Diff #73056)

Didn't you just check that GCRel->getOperand(0) is a statepoint? I think this can be a cast<>

72 ↗(On Diff #73056)

Why not just use the GCRelocateInst::getBasePtr instruction?

This revision now requires changes to proceed.Sep 30 2016, 11:09 AM
anna marked 2 inline comments as done.Sep 30 2016, 12:16 PM

I have some minor comments inline, but overall I'm not sure if a pass like this (solely useful for generating "debugging output") is okay to go upstream.

Do you mind starting a thread on llvm-dev discussing if this sort of thing is okay with the community?

Actually, thats a good point. Especially since RS4GC is not in the pipeline of optimization passes (in PassManagerBuilder.cpp). So, the only usage would be for downstream folks incorporating RS4GC in their optimization pipeline.

lib/Transforms/Utils/RemoveGCRelocates.cpp
69 ↗(On Diff #73056)

yes, I forgot to change that to cast. I had the order of checks other way around initially :)

72 ↗(On Diff #73056)

Ah yes. Although, I'll be using the getDerivedPtr.

anna updated this object.Sep 30 2016, 2:14 PM
anna edited edge metadata.
anna updated this revision to Diff 73146.Sep 30 2016, 2:42 PM
anna edited edge metadata.
anna marked an inline comment as done.

addressed above comments.

anna updated this object.Sep 30 2016, 2:43 PM
anna edited edge metadata.
mehdi_amini added inline comments.
lib/Transforms/Utils/RemoveGCRelocates.cpp
99 ↗(On Diff #73146)

/Bike-shed
What about "strip-gc-relocates" (we already use "strip" for debug info for example and this seems comparable).
/Bike-shed off

reames edited edge metadata.Oct 7 2016, 2:51 PM

(stale comment I never hit submit on)

lib/Transforms/Utils/RemoveGCRelocates.cpp
99 ↗(On Diff #73146)

I like the strip-gc-relocates name as well.

anna updated this revision to Diff 74701.Oct 14 2016, 9:19 AM
anna edited edge metadata.

[StripGCRelocates] Added test cases. Changed pass name to strip-gc-relocates
Please take a look at test5, this is somewhat of a grey area. Do we want to remove the relocates
and add a bit cast? This is the second TODO in the list:
gc.relocate of different type than the original pointer, but all uses are of the same type
as gc.relocate.

anna added a comment.Oct 14 2016, 9:25 AM

Forgot to mention the motivation in continuing the review upstream itself: looks like llvm-dev community is generally in favour, or rather, no objections to adding debugging/diagnostic passes.

http://lists.llvm.org/pipermail/llvm-dev/2016-October/105773.html

sanjoy requested changes to this revision.Oct 19 2016, 1:55 PM
sanjoy edited edge metadata.
sanjoy added inline comments.
lib/Transforms/Utils/RemoveGCRelocates.cpp
36 ↗(On Diff #74701)

Not sure if this is correct, since you're changing IR. I'd suggest starting with "clobbers all" for now.

41 ↗(On Diff #74701)

Why not have this just be a local that lives on stack in runOnFunction?

49 ↗(On Diff #74701)

I didn't know we could have empty non-declarations.

53 ↗(On Diff #74701)

I don't think you need this double staged casting -- you should be able to do:

if (auto *GCR = dyn_cast<GCRelocateInst>(&I))
  GCRelocates.push_back(GCR);
57 ↗(On Diff #74701)

Empty line?

70 ↗(On Diff #74701)

I wouldn't bother with this. I'd just insert a bitcast of the original to i8 addrspace(1)*, RAUW with that. We can always run -instcombine later to clean up the IR quickly.

At the very least, I'd do this ^ in place of the continue down below:

if (GCRel->hasOneUse() && isa<BitCastInst>(GCRel->user_back()))
  ToBeReplaced = GCRel->user_back();
else
  // Create new bitcast and RAUW with that.
This revision now requires changes to proceed.Oct 19 2016, 1:55 PM
anna updated this revision to Diff 75439.Oct 21 2016, 8:54 AM
anna edited edge metadata.
anna marked 4 inline comments as done.

Addressed comments above. Renamed pass name, and handled bit casts.
Tests run with -strip-gc-relocates followed by an instcombine pass to clean up
unnecessary bitcasts.

anna marked an inline comment as done.Oct 21 2016, 8:55 AM
anna added inline comments.
lib/Transforms/Utils/RemoveGCRelocates.cpp
49 ↗(On Diff #74701)

Actually, I think there cannot be empty non-declarations after all optimizations are done.
Either way the pass will not do anything for empty functions. Removed that check.

70 ↗(On Diff #74701)

Yes, the second idea was what I was thinking of originally, and mentioned in comment below.

I like your first idea better, the code is cleaner, and instcombine would get rid of unnecessary bitcasts (if any).

sanjoy accepted this revision.Oct 21 2016, 10:26 AM
sanjoy edited edge metadata.

lgtm with comments

include/llvm/Transforms/Scalar.h
477 ↗(On Diff #75439)

End with period.

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

lib/Transforms/Utils/StripGCRelocates.cpp
11 ↗(On Diff #75439)

s/would be incorrect,/is incorrect,/ sounds better

44 ↗(On Diff #75439)

I'd remove the blank line.

57 ↗(On Diff #75439)

Same here: I'd drop the blank line.

60 ↗(On Diff #75439)

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

This revision is now accepted and ready to land.Oct 21 2016, 10:26 AM
anna marked 6 inline comments as done.Oct 21 2016, 11:32 AM
anna added inline comments.
lib/Transforms/Utils/StripGCRelocates.cpp
60 ↗(On Diff #75439)

That's right. I've updated the comment as well, and avoid tracking Changed.

This revision was automatically updated to reflect the committed changes.