This is an archive of the discontinued LLVM Phabricator instance.

Enable assembly output of local commons for AIX
ClosedPublic

Authored by daltenty on Jul 16 2019, 3:01 PM.

Details

Summary

This patch enable assembly output of local commons for AIX using .lcomm
directives. Adds a EmitXCOFFLocalCommonSymbol to MCStreamer so we can emit the
AIX version of .lcomm assembly directives which include a csect name. Handle the
case of BSS locals in PPCAIXAsmPrinter by using EmitXCOFFLocalCommonSymbol. Adds
a test for generating .lcomm on AIX Targets.

Event Timeline

daltenty created this revision.Jul 16 2019, 3:01 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 16 2019, 3:01 PM
sfertile added inline comments.Jul 29 2019, 12:08 PM
llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
1854–1855

Knowing how the AIX linker treats csects with symbol type XTY_CM, should this be limited to Kind.isLocalBSS() || Kind.isCommon()?

llvm/lib/MC/MCSectionXCOFF.cpp
31–35

ditto.

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1661–1662

I think this also should be limited to isLocalBSS(), we can relax to allowing all BSS in a follow on patch that adds appropriate testing.

llvm/lib/MC/MCSectionXCOFF.cpp
35

"Unsupported" is ambiguous.

This patch needs a rebase. During/after the rebase we need to figure out what the llvm_unreachables should be. That is, the message text needs to clearly express the rationale so that the determination of whether llvm_unreachable is the appropriate mechanism can be done.

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
1858–1860

One or the other of D65159 or this needs to be rebased on the other when one lands.

sfertile added inline comments.Aug 7 2019, 10:59 AM
llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
1858–1860

I'd prefer to get this review moving and commited, then rebase D65159.

daltenty updated this revision to Diff 213961.Aug 7 2019, 11:51 AM

Rebase after commit of dependencies

daltenty marked 2 inline comments as done.Aug 7 2019, 11:53 AM
daltenty updated this revision to Diff 213979.Aug 7 2019, 1:15 PM
Address comments:
  - Limit to local bss
daltenty updated this revision to Diff 213998.Aug 7 2019, 2:18 PM
  • Add missing other cases of isBSSLocal
  • Update unreachable error message to reflect this is an internal error
daltenty marked 4 inline comments as done.Aug 7 2019, 2:19 PM
sfertile accepted this revision.Aug 8 2019, 7:02 AM

LGTM.

llvm/lib/MC/MCSectionXCOFF.cpp
34

minor nit: I think we could shorten common/bss --> '.bss' .

This revision is now accepted and ready to land.Aug 8 2019, 7:02 AM
This revision was automatically updated to reflect the committed changes.
llvm/trunk/include/llvm/MC/MCStreamer.h
547 ↗(On Diff #214163)

Spacing issue.

llvm/trunk/lib/MC/MCAsmStreamer.cpp
762 ↗(On Diff #214163)

Minor nit: Compound adjective "XCOFF-specific" with a hyphen.

768 ↗(On Diff #214163)

Minor nit: Have a period at the end of sentences.

769 ↗(On Diff #214163)

Minor nit: Capitalize the first word of a sentence.

llvm/trunk/lib/MC/MCSectionXCOFF.cpp
34 ↗(On Diff #214163)

I'm not sure that the "unreachables" (in the function currently, not just this one) are buying us enough to warrant the overhead of thinking about them. Generically, the llvm_unreachables in this function are of the type "we are about to do something, and given the check, what we are about to do is not compatible with the input". We believe this to be a error in the internal state of the program that we are not expecting to happen, so assert is okay.

llvm/trunk/lib/MC/MCXCOFFStreamer.cpp
63 ↗(On Diff #214163)

"Not implemented" is not a case where llvm_unreachable is appropriate. Either nothing calls this and it might as well be report_fatal_error, or something does call this and it is probably not the caller's fault that this is unimplemented (as it should be report_fatal_error).

65 ↗(On Diff #214163)

Missing newline at end of file. Windows editor woes?

daltenty marked 7 inline comments as done.Aug 13 2019, 10:17 AM