Page MenuHomePhabricator

[RewriteStatepointsForGC] Strip deref info after rewriting.
ClosedPublic

Authored by sanjoy on May 28 2015, 1:58 PM.

Details

Summary

Once a gc.statepoint has been rewritten to relocate live references, the
SSA values represent physical pointers instead of logical references.
Logical dereferencability does not imply physical dereferencability and
after RewriteStatepointsForGC has run any attributes that imply
dereferencability of the logical references need to be stripped.

This current approach is conservative, and can be made more precise
later if needed. For starters, we need to strip dereferencable
attributes only from pointers that live in the GC address space.

Diff Detail

Repository
rL LLVM

Event Timeline

sanjoy updated this revision to Diff 26734.May 28 2015, 1:58 PM
sanjoy retitled this revision from to [RewriteStatepointsForGC] Strip deref info after rewriting..
sanjoy updated this object.
sanjoy edited the test plan for this revision. (Show Details)
sanjoy added reviewers: reames, pgavlin.
sanjoy added a subscriber: Unknown Object (MLST).
reames edited edge metadata.May 28 2015, 3:43 PM

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?)

lib/Transforms/Scalar/RewriteStatepointsForGC.cpp
2226 ↗(On Diff #26734)

The IfNeeded doesn't add anything in terms of meaning.

2227 ↗(On Diff #26734)

Comment should be outside lambda. I'd be tempted to pull the lambda out as a static helper function since it's fairly large.

2256 ↗(On Diff #26734)

Code duplication ahoy. Could/should this be commoned with a templated helper function?

2275 ↗(On Diff #26734)

Is there a TBAA helper class we can use here? Not sure there is, but please check.

2277 ↗(On Diff #26734)

It looks like you're only handling one of two forms of TBAA metadata here...

2297 ↗(On Diff #26734)

Use Atributes::ReturnIndex both here and above. No more magic constants!

2375 ↗(On Diff #26734)

Whitespace?

sanjoy updated this revision to Diff 26751.May 28 2015, 4:14 PM
sanjoy edited edge metadata.
  • address review
lib/Transforms/Scalar/RewriteStatepointsForGC.cpp
2226 ↗(On Diff #26734)

Will fix.

2227 ↗(On Diff #26734)

Moved out the lambda as a templated static helper.

2256 ↗(On Diff #26734)

Done.

2275 ↗(On Diff #26734)

Looked, could not find anything.

2277 ↗(On Diff #26734)

AFAICT, LLVM's autoupgrade rewrites all TBAA metadata to the struct-path format (in llvm::UpgradeInstWithTBAATag).

2297 ↗(On Diff #26734)

Will fix.

2375 ↗(On Diff #26734)

Will fix.

sanjoy added inline comments.May 28 2015, 4:16 PM
lib/Transforms/Scalar/RewriteStatepointsForGC.cpp
2275 ↗(On Diff #26734)

To clarify -- I could not find a public interface for the TBAA metadata. There is a helper class inside TypeBasedAliasAnalysis.cpp that's not exported.

reames accepted this revision.Jun 1 2015, 3:40 PM
reames edited edge metadata.

LGTM. Still not crazy about the fact we have to do this, but the actual implementation seems reasonable and I can't come up with a better approach.

This revision is now accepted and ready to land.Jun 1 2015, 3:40 PM
This revision was automatically updated to reflect the committed changes.