This is an archive of the discontinued LLVM Phabricator instance.

[RewriteStatepointsForGC] Extend base pointer to handle more cases w/vectors
ClosedPublic

Authored by reames on May 11 2015, 1:14 PM.

Details

Summary

When relocating a pointer, we need to determine a base pointer for the derived pointer being relocated. We have limited support for handling a pointer extracted from a vector; the current code only handled the case where the entire vector was known to contain base pointers. This patch extends the reasoning to handle chains of insertelements where the indices are constants. This case turns out to be fairly common in vectorized code. We can now handle vectors which contains mixtures of base and derived pointers provided the insertelements use constant indices.

Note that this doesn't solve the general problem. To handle variable indexed insertelements, we'd need to scalarize and introduce conditional branching based on the index. Alternatively, we could eagerly scalarize, but the code structure doesn't currently make either fix easy. The patch also doesn't handle shufflevector or other vector manipulation for much the same reasons. I plan to defer this work until I have a motivating test case.

Diff Detail

Repository
rL LLVM

Event Timeline

reames updated this revision to Diff 25499.May 11 2015, 1:14 PM
reames retitled this revision from to [RewriteStatepointsForGC] Extend base pointer to handle more cases w/vectors.
reames updated this object.
reames edited the test plan for this revision. (Show Details)
reames added a subscriber: Unknown Object (MLST).
pgavlin edited edge metadata.May 11 2015, 4:21 PM

LGTM with one nit.

lib/Transforms/Scalar/RewriteStatepointsForGC.cpp
304 ↗(On Diff #25499)

Isn't Index != InsertIndex redundant?

pgavlin accepted this revision.May 11 2015, 4:22 PM
pgavlin edited edge metadata.
This revision is now accepted and ready to land.May 11 2015, 4:22 PM
reames added inline comments.May 12 2015, 10:15 AM
lib/Transforms/Scalar/RewriteStatepointsForGC.cpp
304 ↗(On Diff #25499)

Technically, yes, but I think the code would be far less obvious without it. I'd prefer to leave it. Do you object?

pgavlin added inline comments.May 12 2015, 10:18 AM
lib/Transforms/Scalar/RewriteStatepointsForGC.cpp
304 ↗(On Diff #25499)

Somewhat: redundant checks tend to leave me wondering whether or not I missed a change between the checks. Personal preference would be to remove the redundant check, but I don't feel too strongly here.

This revision was automatically updated to reflect the committed changes.