[PATCH] D83252: [XCOFF] Enable symbol alias for AIX

[Jason Liu via Phabricator via llvm-commits]

jasonliu marked 7 inline comments as done.
jasonliu added inline comments.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:1408
+    assert(!isa<GlobalIFunc>(GIS) && "IFunc is not supported on AIX.");
+    assert(MAI->hasVisibilityOnlyWithLinkage() &&
+           "Visibility should be handled with emitLinkage() on AIX.");
----------------
Xiangling_L wrote:
> 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.
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.


================
Comment at: llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp:2152
 MCSymbol *TargetLoweringObjectFileXCOFF::getFunctionEntryPointSymbol(
-    const Function *F, const TargetMachine &TM) const {
+    const GlobalValue *Function, const TargetMachine &TM) const {
   SmallString<128> NameStr;
----------------
DiggerLin wrote:
> 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 ?
Sounds good. 


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1841
+          "alias without a base object is not yet supported on AIX");
+    GOAliasMap[Base].push_back(&Alias);
+  }
----------------
DiggerLin wrote:
> if Base ->isDeclarationForLinker() is true. it should not have a alias, do we need to add assert here ?
Is it necessary? We already get both clang and llc errors if we try to do that.


================
Comment at: llvm/test/CodeGen/PowerPC/aix-alias.ll:24
+ at bar_p = protected alias i32, i32* @bar
+ at fun_ptr = global i32()* @bar_f, align 4
+
----------------
DiggerLin wrote:
> Xiangling_L wrote:
> > You can simplify the testcase by removing `align 4` all over the test.
> 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?
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.


================
Comment at: llvm/test/CodeGen/PowerPC/aix-alias.ll:72
+; ASM-NEXT:    .globl  foo2
+; ASM-NEXT:    .weak bar_f
+; ASM-NEXT:    .weak .bar_f
----------------
Xiangling_L wrote:
> I am wondering any reason we want to separate the linkage emission with the alias label emission?
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.


================
Comment at: llvm/test/CodeGen/PowerPC/aix-alias.ll:86
+; ASM-NEXT:  L..C3:
+; ASM-NEXT:    .tc bar_f[TC],bar_f
----------------
Xiangling_L wrote:
> I am suggesting to also test the usage of hidden and protected alias.
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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83252/new/

https://reviews.llvm.org/D83252