Page MenuHomePhabricator

[XCOFF] Enable -fdata-sections on AIX

Authored by jasonliu on Sep 25 2020, 2:08 PM.



Some design decision worth noting about:

I've noticed a recent mailing discussing about why string literal is not affected by -fdata-sections for ELF target:

But on AIX, our linker could not split the mergeable string like other target.
So I think it would make more sense for us to emit separate csect for every mergeable string in -fdata-sections mode,
as there might not be other ways for linker to do garbage collection on unused mergeable string.

Diff Detail

Event Timeline

jasonliu created this revision.Sep 25 2020, 2:08 PM
jasonliu requested review of this revision.Sep 25 2020, 2:08 PM
jasonliu edited the summary of this revision. (Show Details)
daltenty added inline comments.Sep 28 2020, 7:52 AM

We're currently gating this behavior on the code-gen data-sections option, but AIX common initialization does really function correctly without it, so we should probably adopt this as a default.

Summarizing from off-list discussion:

Variables with common linkage will be emitted as a common-type csect in XCOFF. Per the assembly reference:

"Several modules can share the same common block. If any of those modules have an external Control Section (csect) with the same name and the csect with the same name has a storage mapping class other than BS or UC, then the common block is initialized and becomes that other Control Section.”

Initialization via a non-csect (i.e label-ed data) is not defined, and results in a linker warning:

$ cat common.s
.comm foo, 2
$ cat label.s
.csect data[RW]
.globl foo
        .vbyte 2,0
.globl __start
$ ld common.o label.o
ld: 0711-224 WARNING: Duplicate symbol: foo
ld: 0711-345 Use the -bloadmap or -bnoquiet option to obtain more information.
jasonliu added inline comments.Sep 28 2020, 12:12 PM

Summarizing discussion offline:
Instead of making it a default and disallow user to specify -fno-data-sections on AIX, it's still worth providing user that option to turn it off when they know what they are doing and they think the linker is behaivoring correctly in their case.
But we should make the llc option -data-sections=true on AIX by default instead, so that we get the correct behaivor out of the box from llc. And for that, I will post a separate patch for it.

daltenty added inline comments.Sep 29 2020, 8:58 AM

Probably check data sections first (Consider adding LLVM_LIKELY in next patch)

jasonliu updated this revision to Diff 295026.Sep 29 2020, 10:02 AM
jasonliu marked an inline comment as done.
jasonliu added inline comments.

Thanks. Will do.

daltenty accepted this revision.Sep 30 2020, 9:09 AM

LGTM, thanks


Note that this might be changed by D80953


nit: I think we could drop the instruction decode from the check since it's not meaningful and we're actually only testing the raw data

This revision is now accepted and ready to land.Sep 30 2020, 9:09 AM
jasonliu updated this revision to Diff 295397.Sep 30 2020, 1:35 PM
jasonliu marked an inline comment as done.

Rebase and addresses comments.

jasonliu marked 2 inline comments as done.Sep 30 2020, 1:35 PM

Should the scope cover the constant pool?

The output for llvm/test/CodeGen/PowerPC/aix-lower-constant-pool-index.ll appears to still use a single .rodata csect for items in the constant pool.

jasonliu added inline comments.Oct 1 2020, 12:54 PM

Summarize offline discussions:
It’s actually not trivial to change the constant section emission:
This is the function declaration for it:

MCSection *getSectionForConstant(const DataLayout &DL, SectionKind Kind,
                                 const Constant *C,
                                 Align &Alignment) const override;

There is no existing way to mangle the Constant. And it could be a nullptr for all we know.
We could try to roll our own way to mangle a Constant, but at this point, it might not worth the effort.

jasonliu added inline comments.Oct 1 2020, 2:27 PM

I'm planning to land this as is. Let me know if that's okay with you.


Works for me. Thanks.

This revision was automatically updated to reflect the committed changes.