Page MenuHomePhabricator

Consider offset in global variables during lowering in NVPTX.
ClosedPublic

Authored by sfantao on May 22 2015, 11:03 AM.

Details

Reviewers
jholewinski
Summary

If the NVPTX backend is used with relocation model 'static', DAGCombiner will have global addresses combined with any add instruction that adds a constant offset to it. However, lowering is not prepared to deal with the offset that is added in GlobalAddress nodes. This issue can be replicated if llc is called with relocation-model=static or if the backend is invoked directly from clang.

This patch fixes the issue by undoing what DAGCombiner does, which looks to me as the less disruptive solution. Next to the fix, a comment is added explaining other alternatives to tackle this issue.

Thanks!
Samuel

Diff Detail

Event Timeline

sfantao updated this revision to Diff 26331.May 22 2015, 11:03 AM
sfantao retitled this revision from to Consider offset in global variables during lowering in NVPTX..
sfantao updated this object.
sfantao edited the test plan for this revision. (Show Details)
sfantao added a reviewer: jholewinski.
sfantao added a subscriber: Unknown Object (MLST).
jholewinski edited edge metadata.Jun 8 2015, 3:50 PM

What is the purpose to using a static relocation model for NVPTX? I would prefer to just enforce a default here, especially since this is essentially unused for NVPTX.

sfantao updated this revision to Diff 27380.Jun 9 2015, 10:04 AM
sfantao edited edge metadata.

This diff changes the original fix to enforce the default relocation model to be used regardless of what the client specifies.

My understanding is that the backend clients (e.g. clang) may request a given relocation model. If nothing else is specified by the user, this relocation model would be 'static'. I thought relocation model = default would map to one of the supported relocation models, and static would be what makes more sense for NVPTX. This would however drive some optimized behaviors in the combiner causing the issue reported here to be exposed.

The diff above discards any relocation model information specified by the client.

Let me know any other comments you may have,

Thanks!
Samuel

Any more comments on this patch?

Thanks!
Samuel

Any other comments about this patch?

Thanks,
Samuel

jholewinski accepted this revision.Jun 30 2015, 9:17 AM
jholewinski edited edge metadata.

Sounds reasonable to me.

This revision is now accepted and ready to land.Jun 30 2015, 9:17 AM
sfantao closed this revision.Jun 30 2015, 10:18 AM

Committed in r241081.

Thanks!
Samuel