Page MenuHomePhabricator

[XCOFF] Enable symbol alias for AIX
ClosedPublic

Authored by jasonliu on Jul 6 2020, 12:37 PM.

Details

Summary

AIX assembly's .set directive is not usable for aliasing purpose.
We need to use extra-label-at-defintion strategy to generate symbol aliasing on AIX.

Follow up items after this patch would be:

  1. Investigate .set directive to see if it's needed for other purpose.
  2. Use llvm-readobj to dump the relocation table and symbol table for the symbols to verify it on the integrate-as path.

Diff Detail

Event Timeline

jasonliu created this revision.Jul 6 2020, 12:37 PM

Gentle ping.

Xiangling_L added inline comments.Jul 15 2020, 8:54 AM
llvm/include/llvm/Target/TargetLoweringObjectFile.h
250–251

Instead of comments, can we add an assertion for this?

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
1408

I don't quite get the point of this assertion here, it seems hasVisibilityOnlyWithLinkage() will always be true if this is an XCOFF target which is exactly same as the if condition used here. Maybe a comment Visibility should be handled with emitLinkage() on AIX. is sufficient.

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
2129

The same comments as above, can we add assertions here saying this only applies for GlobalObject of GlobalAlias?

llvm/test/CodeGen/PowerPC/aix-alias.ll
1–2

minor nit:
trailing whitespace

18

I feel the naming convention here is kinda messy. It seems there are mainly two kind, one is aliasing to a variable bar, the other is aliasing to a function foo_f.
I am suggesting to stick bar related alias, with bar_xxx and prabably use foo_f_xxx to refer to foo_f related alias.
[eg.
@foo_f_w = weak alias %FunTy, %FunTy* @foo_f
]

24

You can simplify the testcase by removing align 4 all over the test.

72

I am wondering any reason we want to separate the linkage emission with the alias label emission?

86

I am suggesting to also test the usage of hidden and protected alias.

DiggerLin added inline comments.Jul 15 2020, 9:15 AM
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1851

if Base ->isDeclarationForLinker() is true. it should not have a alias, do we need to add assert here ?

llvm/test/CodeGen/PowerPC/aix-alias.ll
24

in the doc .https://llvm.org/docs/LangRef.html#aliases . The linkage must be one of private, internal, linkonce, weak, linkonce_odr, weak_odr, external.

Do you want to test the linkonce, weak_odr,and external by the way?

DiggerLin added inline comments.Jul 15 2020, 10:55 AM
llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
2154–2160

I saw that you change the const GlobalValue *Function to
const GlobalValue *Func, in the patch https://reviews.llvm.org/D83875 why not change in the patch here directly ?

DiggerLin added inline comments.Jul 15 2020, 11:09 AM
llvm/test/CodeGen/PowerPC/aix-alias.ll
49

for the convenient of review . I suggest that adding two line
;ASM-NEXT: # %bb.0:
;ASM-NEXT: li 3, 0

and the reviewer will know the
.bar_f and .foo_f pointer to different position with following .test: (all two more CHECK line for .test below)

jasonliu marked 7 inline comments as done.Jul 15 2020, 1:00 PM
jasonliu added inline comments.
llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
1408

The assertion here is essentially a documentation. I'd prefer to let the assertion to tell the story and comments are easier to get ignored.

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
2154–2160

Sounds good.

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

Is it necessary? We already get both clang and llc errors if we try to do that.

llvm/test/CodeGen/PowerPC/aix-alias.ll
24

Is there anything special about linkonce, weak_odr and external that would trigger a different behavior between aliasing and the actual object to make it worth testing? Additional linkage does not have any difference for emitLinkage except using label vs csect case when dealing with aliasing vs actual object.

72

We could do it with the alias label emission as well. But doing it here follows what llvm does currently for other targets. And also maybe later when we decide to handle aliasing without base global object, we do not need to filter out all the aliases that already have their linkage emitted somewhere else.

86

I think it's worth to test the function aliasing with one of the visibility attribute. i.e. hidden.
I don't think it's necessary to test all the combinations, as for aliasing purpose, they all go through similar path there. Extensive testing of visibility is already handled in visibility patch.

DiggerLin added inline comments.Jul 15 2020, 1:02 PM
llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
1406

change if (TM.getTargetTriple().isOSBinFormatXCOFF() )
to
if (TM.getTargetTriple().isOSBinFormatXCOFF() & IsFunction)

and we do no not need
if (IsFunction)
later

jasonliu marked an inline comment as done.Jul 15 2020, 1:08 PM
jasonliu added inline comments.
llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
1406

I'm not sure how that would work because this if statement also intends to handle GlobalVariable.

jasonliu marked an inline comment as done.Jul 15 2020, 1:09 PM
jasonliu added inline comments.
llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
1406

I mean alias to GlobalVariable. Not just Function.

DiggerLin added inline comments.Jul 15 2020, 1:29 PM
llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
1406

yes, you are correct. the emitGlobalIndirectSymbol also been called for the GlobalObject too.

DiggerLin added inline comments.Jul 15 2020, 1:31 PM
llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
1406

sorry, the emitGlobalIndirectSymbol also been called for the GlobalVariable too.

jasonliu updated this revision to Diff 278321.Jul 15 2020, 2:56 PM
jasonliu marked 6 inline comments as done.

Address comments.

llvm/test/CodeGen/PowerPC/aix-alias.ll
18

Agree. Renamed and reorganized the test.

jasonliu updated this revision to Diff 278323.Jul 15 2020, 3:04 PM
Xiangling_L added inline comments.Jul 15 2020, 5:30 PM
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1847

I am suggesting we add tests also for alias to alias and either one scenario of alias to ConstantExpr.

llvm/test/CodeGen/PowerPC/aix-alias.ll
86

Yeah, agree. That works as well.

jasonliu updated this revision to Diff 278511.Jul 16 2020, 9:10 AM

Update test case to include more scenario.

jasonliu marked an inline comment as done.Jul 16 2020, 9:10 AM
This revision is now accepted and ready to land.Jul 17 2020, 8:58 AM
Xiangling_L accepted this revision.Jul 20 2020, 7:58 AM

LGTM with minor nit.

llvm/test/CodeGen/PowerPC/aix-alias.ll
19

Remove align 4 on #19 and #20?

This revision was automatically updated to reflect the committed changes.
jasonliu marked an inline comment as done.Jul 22 2020, 7:07 AM
jasonliu added inline comments.
llvm/test/CodeGen/PowerPC/aix-alias.ll
19

As discussed offline, the align 4 is generated by clang. It's not very intrusive and does not make the IR significantly hard to read. We could keep it here.