Page MenuHomePhabricator

[PowerPC][AIX] Adds support for writing the .data section in assembly files
ClosedPublic

Authored by xingxue on Aug 13 2019, 10:39 AM.

Details

Summary

Adds support for generating the .data section in assembly files for global variables with a non-zero initialization. The support for writing the .data section in XCOFF object files will be added in a follow-on patch. Any relocations are not included in this patch.

Diff Detail

Event Timeline

xingxue created this revision.Aug 13 2019, 10:39 AM
xingxue updated this revision to Diff 216404.Aug 21 2019, 8:02 AM
llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
165–167

This does not need to be a non-static member function. It can be a static member function.

llvm/lib/MC/MCAsmStreamer.cpp
1122

Given the code here, the name of the function (and the variable) is misleading. It should say useDotAlignForAlignment, because we don't fall back to anything else when the requested alignment is not a power of two.

1123

With the suggestion to change the error text below, I think we can remove the comment.

1125

Suggestion: "Only power-of-two alignments are supported with .align."

llvm/lib/MC/MCSectionXCOFF.cpp
33

What we should write here, and whether llvm_unreachable is appropriate depends. I think that the current patch allows us to say that we do not handle switching to data sections with other storage mapping classes yet. That can be an assert, but probably not an unreachable.

assert(getMappingClass() == XCOFF::XMC_RW &&
       "Unhandled storage-mapping class for data section.");
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1662–1664

Stray change from isBSSLocal to isBSS. Please fix.

1663

Suggestion: "Encountered a global variable kind that is not supported yet."

llvm/test/CodeGen/PowerPC/aix-xcoff-data.ll
26

We should seek to understand why GCC and XL uses 4-byte alignment.

xingxue updated this revision to Diff 216660.Aug 22 2019, 12:28 PM

Addressed comments:

  • Rename function useDotAlignForPowerOfTwoAlignment() to useDotAlignForAlignment() and the associated variable from UseDotAlignForPowerOfTwoAlignment to UseDotAlignForAlignment.
  • Define AsmPrinter::getGVAlignmentLog2() as a static member function.
  • Change from calling llvm_unreachable() to assert() as suggested for unsupported storage-mapping class of the data section.
  • Fix the condition of supported global variable kind.
xingxue marked 7 inline comments as done.Aug 22 2019, 12:30 PM
xingxue added inline comments.
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1663

Good catch, thanks!

A few small changes, but otherwise LGTM.

llvm/include/llvm/MC/MCAsmInfo.h
168

Suggestion: "True if .align is to be used for alignment. Only power-of-two alignment is supported."

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1678–1681

Add a newline before the comment.

1679

I think this should be isBSSLocal.

This revision is now accepted and ready to land.Aug 22 2019, 3:45 PM
xingxue updated this revision to Diff 216751.Aug 22 2019, 6:27 PM
xingxue marked an inline comment as done.

Addressed comments:

  • Changed the comment for variable UseDotAlignForAlignment as suggested.
  • Replaced condition GVKind.isBSS() with GVKind.isBSSLocal().
  • Added a new line.
xingxue marked 4 inline comments as done.Aug 22 2019, 6:32 PM
This revision was automatically updated to reflect the committed changes.
xingxue updated this revision to Diff 217045.Aug 25 2019, 8:43 AM

Merged with the master and fixed a typo.