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

[Xiangling Liao via Phabricator via llvm-commits]

Xiangling_L added inline comments.


================
Comment at: llvm/include/llvm/Target/TargetLoweringObjectFile.h:250
   /// Otherwise, returns nulltpr.
-  virtual MCSymbol *getFunctionEntryPointSymbol(const Function *F,
+  /// GV must be a function or an alias which has a function as base object.
+  virtual MCSymbol *getFunctionEntryPointSymbol(const GlobalValue *F,
----------------
Instead of comments, can we add an assertion for this?


================
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.");
----------------
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.


================
Comment at: llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp:2129
+XCOFF::StorageClass
+TargetLoweringObjectFileXCOFF::getStorageClassForGlobal(const GlobalValue *GV) {
+  switch (GV->getLinkage()) {
----------------
The same comments as above, can we add assertions here saying this only applies for GlobalObject of GlobalAlias?


================
Comment at: llvm/test/CodeGen/PowerPC/aix-alias.ll:1
-; RUN: not --crash llc < %s -mtriple powerpc-ibm-aix-xcoff 2>&1 | FileCheck %s
-; RUN: not --crash llc < %s -mtriple powerpc64-ibm-aix-xcoff 2>&1 | FileCheck %s
+; TODO: Add object generation test when visibility for object generation 
+;       is implemnted.
----------------
minor nit:
trailing whitespace


================
Comment at: llvm/test/CodeGen/PowerPC/aix-alias.ll:18
+}
+
+ at bar_f = weak alias %FunTy, %FunTy* @foo_f
----------------
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
]


================
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
+
----------------
You can simplify the testcase by removing `align 4` all over the test.


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


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


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

https://reviews.llvm.org/D83252