This is an archive of the discontinued LLVM Phabricator instance.

[XCOFF] change default program code csect alignment to 32
ClosedPublic

Authored by shchenz on Nov 23 2021, 1:27 AM.

Details

Summary

This is the same with commercial XLC on AIX. And Some internal improvements were found after this change.

There are some follow-ups:
1: for commercial XLC on AIX, the default csect alignment for read-write data section is 4 bytes for 32bit XCOFF and 8 bytes for 64 bit XCOFF. For llvm on AIX, now they are both 4 bytes.
2: For the XCOFF section that has one entry in the section header, commercial XLC on AIX also use 32 bytes as alignment, but now in XCOFFObjectWriter.cpp, the alignment is also 4 bytes.

Diff Detail

Event Timeline

shchenz created this revision.Nov 23 2021, 1:27 AM
shchenz requested review of this revision.Nov 23 2021, 1:27 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 23 2021, 1:27 AM

Just out of curiosity, if the csect that contains a function is aligned at 2 bytes, how do we ensure loops within those functions are aligned at 32 bytes?

shchenz updated this revision to Diff 390011.Nov 26 2021, 4:40 AM
shchenz retitled this revision from [WIP] [XCOFF] set default csect alignment to 32 to [XCOFF] change default program code csect alignment to 32.
shchenz edited the summary of this revision. (Show Details)
shchenz added reviewers: sfertile, hubert.reinterpretcast, jsji, nemanjai, Restricted Project.

Just out of curiosity, if the csect that contains a function is aligned at 2 bytes, how do we ensure loops within those functions are aligned at 32 bytes?

I am not checking the details, but if a function has a loop that is aligned at 32bytes, will the function be aligned at 2 bytes? I think the function alignment should be no smaller than its nested loop's alignment?

And there is a logic in the csect alignment setting that the csect alignment must be no smaller than its nested function's alignment. See

AsmPrinter::emitFunctionHeader() {
  if (MAI->hasFunctionAlignment())
    emitAlignment(MF->getAlignment(), &F)
}

gentle ping

gentle ping

jsji added a comment.Dec 13 2021, 11:05 AM

Do we have any data about the *size* change due to alignment change? especially for code with lots of small functions, like perlbench?

Do we have any data about the *size* change due to alignment change? especially for code with lots of small functions, like perlbench?

I tested perlbench on AIX with -ffunction-sections and found about 1% size increase.
I also tested some other benchmarks, and the size increments are all in 1% ~ 2% scope.

jsji added a comment.Dec 16 2021, 8:25 AM

Can you try adding a testcase for Os or Oz to see what happens? I suspect that this change might override the pref alignment settings in Lowering.

Also have you tried to gather the performance by changing setPrefLoopAlignment(16) to setPrefLoopAlignment(32) for AIX in PPCISelLowering.cpp?
I suspect most of the performance improvement you get should be from chanig loop alignment for *large* loops -- we only set inner most small loops to 32 for now.

Can you try adding a testcase for Os or Oz to see what happens? I suspect that this change might override the pref alignment settings in Lowering.

There is a case test_minsize() in file test/CodeGen/PowerPC/code-align.ll, there is no change for that case, so I think there should be no change for the loop alignment after this patch?

Also have you tried to gather the performance by changing setPrefLoopAlignment(16) to setPrefLoopAlignment(32) for AIX in PPCISelLowering.cpp?
I suspect most of the performance improvement you get should be from chanig loop alignment for *large* loops -- we only set inner most small loops to 32 for now.

After changing setPrefLoopAlignment(16) to setPrefLoopAlignment(32) (with/without setPrefFunctionAlignment(32)), there is no improvements found for the benchmarks that this patch improves.

Function/Loop alignment setting in PPCISelLowering.cpp should be an input to the csect alignment setting in AsmPrinter. AsmPrinter must ensure the csect alignment is suitable for the Function/Loop alignments.

AsmPrinter::emitFunctionHeader() {
  if (MAI->hasFunctionAlignment())
    emitAlignment(MF->getAlignment(), &F)
}
void MCObjectStreamer::emitValueToAlignment(unsigned ByteAlignment,
                                            int64_t Value,
                                            unsigned ValueSize,
                                            unsigned MaxBytesToEmit) {
  if (MaxBytesToEmit == 0)
    MaxBytesToEmit = ByteAlignment;
  insert(new MCAlignFragment(ByteAlignment, Value, ValueSize, MaxBytesToEmit));

  // Update the maximum alignment on the current section if necessary.
  MCSection *CurSec = getCurrentSectionOnly();
  if (ByteAlignment > CurSec->getAlignment())
    CurSec->setAlignment(Align(ByteAlignment));
}
jsji added inline comments.Dec 17 2021, 7:43 AM
llvm/lib/MC/XCOFFObjectWriter.cpp
1050 ↗(On Diff #390011)

these are unrelated to the default alignment change? If we change the text alignment with -align-all-functions , we also need to deal with these? If so, please split these changes into another patch.

shchenz added inline comments.Dec 18 2021, 5:02 AM
llvm/lib/MC/XCOFFObjectWriter.cpp
1050 ↗(On Diff #390011)

Right, if the DWARF csect alignment is the same as the section alignment defined in this file(DefaultSectionAlign=4), we are ok. But if they are not the same, we will have some issue. This is to fix this issue. I will split them into another patch.

Herald added a project: Restricted Project. · View Herald TranscriptMar 13 2022, 6:11 PM
jsji resigned from this revision.Jun 2 2022, 7:51 AM

gentle ping

Esme added a comment.Jun 22 2022, 6:27 PM

I have no more comments other than some nits.

llvm/include/llvm/MC/MCSectionXCOFF.h
62
63
llvm/test/CodeGen/PowerPC/aix-emit-tracebacktable-clobber-register.ll
92–93
llvm/test/CodeGen/PowerPC/aix-emit-tracebacktable.ll
177
Esme added a comment.Jun 22 2022, 6:33 PM

BTW, some tests failed in pre-merge checks, and it might be related to this patch?

shchenz updated this revision to Diff 439613.Jun 23 2022, 9:21 PM
shchenz marked 4 inline comments as done.

address @Esme comments

Thanks for review @Esme . Updated accordingly.

Esme accepted this revision.Jun 27 2022, 6:59 PM

LGTM, thx.

This revision is now accepted and ready to land.Jun 27 2022, 6:59 PM
This revision was landed with ongoing or failed builds.Jun 28 2022, 9:17 PM
This revision was automatically updated to reflect the committed changes.