Page MenuHomePhabricator

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

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



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

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


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.


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


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


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.");

Stray change from isBSSLocal to isBSS. Please fix.


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


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.

Good catch, thanks!

A few small changes, but otherwise LGTM.


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


Add a newline before the comment.


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.