This is an archive of the discontinued LLVM Phabricator instance.

[AIX] Make sure to use QualNames for external global objects
ClosedPublic

Authored by daltenty on Dec 4 2019, 1:53 PM.

Details

Summary

Previously we only handled the case where the csect hadn't been set up yet, so we'd hit an assert later on.

Event Timeline

daltenty created this revision.Dec 4 2019, 1:53 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 4 2019, 1:54 PM
hubert.reinterpretcast edited the summary of this revision. (Show Details)Dec 4 2019, 3:46 PM
hubert.reinterpretcast added inline comments.
llvm/test/CodeGen/PowerPC/aix-xcoff-toc.ll
31

This needs a comment to explain why a second function is useful for the test. The fact that this tests for a bug that need not have existed is non-obvious.

86

It is possible to use an expression here that is based on the value observed for the TOC base? Similarly for the lines below.

daltenty marked an inline comment as done.Dec 4 2019, 8:13 PM
daltenty added inline comments.
llvm/test/CodeGen/PowerPC/aix-xcoff-toc.ll
86

I believe doing so is dependent on the hex format support under review in D60389

jasonliu added inline comments.Dec 5 2019, 7:32 AM
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1897

nit: This return statement is like a short cut, maybe we could remove it to make this if block simpler?

daltenty updated this revision to Diff 232345.Dec 5 2019, 7:34 AM
  • Add comment
daltenty marked an inline comment as done.Dec 5 2019, 7:34 AM
daltenty updated this revision to Diff 232352.Dec 5 2019, 8:07 AM
  • Remove redundant return
daltenty marked an inline comment as done.Dec 5 2019, 8:07 AM
jasonliu accepted this revision.Dec 5 2019, 9:00 AM

LGTM with minor nit.

llvm/test/CodeGen/PowerPC/aix-xcoff-toc.ll
31

Suggestion for comment:
Reference external globals again to make sure setting up csect for external global is properly guarded.

This revision is now accepted and ready to land.Dec 5 2019, 9:00 AM
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1888

How do we know that having the containing csect set is enough to know that the storage class has been set? Should we have an assume after the if block that the storage class is set? Or perhaps a comment that the containing csect could not have been set without the storage class being set on the symbol.

This revision was automatically updated to reflect the committed changes.