This is an archive of the discontinued LLVM Phabricator instance.

[NVPTX] Avoid dots in global names
ClosedPublic

Authored by asavonic on Apr 4 2022, 8:35 AM.

Details

Summary

It seems that ptxas cannot parse them:
ptxas fatal: Parsing error near '.2': syntax error

Diff Detail

Event Timeline

asavonic created this revision.Apr 4 2022, 8:35 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 4 2022, 8:35 AM
asavonic requested review of this revision.Apr 4 2022, 8:35 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 4 2022, 8:35 AM
tra added a comment.Apr 4 2022, 11:23 AM

Normally such replacement is done with nvptx-assign-valid-global-names pass, so we could pipe the tests through opt -nvptx-assign-valid-global-names which will ensure that tests don't have to know that dot is special for NVPTX. The downside is that it would have to be done for all llc tests. Perhaps we should consider moving the nvptx-assign-valid-global-names pass to the back-end.

Not using dots only fixes the issues we have now and would still leave us open to test breaks if a new NVPTX-invalid name is added by accident. I think running nvptx-assign-valid-global-names in llc may be a better solution.

Normally such replacement is done with nvptx-assign-valid-global-names pass, so we could pipe the tests through opt -nvptx-assign-valid-global-names which will ensure that tests don't have to know that dot is special for NVPTX. The downside is that it would have to be done for all llc tests. Perhaps we should consider moving the nvptx-assign-valid-global-names pass to the back-end.

Not using dots only fixes the issues we have now and would still leave us open to test breaks if a new NVPTX-invalid name is added by accident. I think running nvptx-assign-valid-global-names in llc may be a better solution.

NVPTXAssignValidGlobalNames should already run in llc, but it can only rename globals with local linkage. Globals with external linkage are not renamed, and I'm not sure what is the rationale here.

tra accepted this revision.Apr 12 2022, 11:19 AM

LGTM. Automatic renaming should be addressed separately, anyways.

This revision is now accepted and ready to land.Apr 12 2022, 11:19 AM
This revision was landed with ongoing or failed builds.Apr 14 2022, 7:08 AM
This revision was automatically updated to reflect the committed changes.