Page MenuHomePhabricator

[XCOFF][AIX] Emit correct alignment for csect
ClosedPublic

Authored by jasonliu on Apr 29 2020, 1:52 PM.

Details

Summary

This patch tries to emit the correct alignment result for both object file generation path and assembly path.

Diff Detail

Event Timeline

jasonliu created this revision.Apr 29 2020, 1:52 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 29 2020, 1:52 PM
jasonliu edited the summary of this revision. (Show Details)Apr 29 2020, 1:55 PM
jasonliu updated this revision to Diff 261024.Apr 29 2020, 1:57 PM
DiggerLin added inline comments.May 4 2020, 8:33 AM
llvm/lib/MC/MCContext.cpp
624 ↗(On Diff #261024)

how about to define static const expr DefaultAlign=4 ; in the MCSectionXCOFF class ?

llvm/lib/MC/MCXCOFFStreamer.cpp
66

do we need to add safeguard of cast<MCSymbolXCOFF>(Symbol)->hasRepresentedCsect() here ?

jasonliu marked an inline comment as done.May 4 2020, 10:11 AM
jasonliu added inline comments.
llvm/lib/MC/MCXCOFFStreamer.cpp
66

No, common symbol must be a csect, so it should have represented csect.

daltenty added inline comments.May 4 2020, 1:52 PM
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1723

I'm not clear why we need this check here? Trying to SelectionSectionForGlobal will already give us an error if either Function/Data sections are on. Is there a unique reason that this approach won't work with data-sections? If so, a comment is probably in order.

jasonliu updated this revision to Diff 262651.May 7 2020, 7:37 AM
jasonliu marked 3 inline comments as done.

Address comments.

llvm/lib/MC/MCContext.cpp
624 ↗(On Diff #261024)

Thanks. Just realized I can do this in MCSectionXCOFF constructor as well.

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1723

Thanks. Added reasoning in the comment.

This revision is now accepted and ready to land.May 7 2020, 9:56 AM
llvm/test/CodeGen/PowerPC/aix-readonly-with-relocation.ll
8

Should the style be consistent between .csect and .comm over the use of spaces after the comma?

llvm/lib/MC/MCXCOFFStreamer.cpp
63

Suggest newline before the comment.

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1724

Even if data sections are used, explicitly-specified sections could be used to group variables together.

1742

s/know csect's alignment upfront/know, up front, the alignment of csects/;
Add "the" before "assembly path".

1743

Add "a" before ".csect directive".

1748

Use the immediate base class even if it does not currently override this function.

llvm/test/CodeGen/PowerPC/test_func_desc.ll
6

It seems we have nothing to verify alignment of functions:

__attribute__((__aligned__(32))) int bar() { return 0; }
jasonliu updated this revision to Diff 262943.May 8 2020, 1:23 PM
jasonliu marked 6 inline comments as done.

Add in support for function alignment.
Move the logic from doFinialization to doInitialization because ".text" csect is emitted very early.

llvm/test/CodeGen/PowerPC/aix-readonly-with-relocation.ll
8

I agree the style should be consistent for the use of spaces after comma.
It seems like for clang, we would have a space after comma (at least for instructions). So looks like .csect directive is following that convention.
We could have a NFC patch later to clean it up for .comm.

LGTM with minor nits. Thanks.

llvm/include/llvm/MC/MCSectionXCOFF.h
51

s/Csect/A csect/;
Add "csects" after "undefined symbol".

llvm/lib/MC/MCXCOFFStreamer.cpp
64

s/value/values/;

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1723

LLVM naming convention: Result.

llvm/test/CodeGen/PowerPC/aix-readonly-with-relocation.ll
8

Okay; thanks.

llvm/test/CodeGen/aix-func-align.ll
2

s/the csect contains/a csect containing/;

11

Indent the FileCheck invocation. From D78929, it looks like two spaces.

This revision was automatically updated to reflect the committed changes.
jasonliu marked 6 inline comments as done.

Fixed a build break introduced by this change in https://github.com/llvm/llvm-project/commit/a1b04aaea210a9a9fbe0cd9dd7f874e12fa97585
I should've put aix-func-align.ll in the PowerPC sub-directive.