[PATCH] D75866: [AIX] supporting the visibility attribute for aix assembly

[Digger via Phabricator via llvm-commits]

DiggerLin marked 27 inline comments as done.
DiggerLin added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1587
+  case GlobalValue::DefaultVisibility:
+    PPCAsmPrinter::emitLinkage(GV, GVSym);
+    return;
----------------
jasonliu wrote:
> DiggerLin wrote:
> > jasonliu wrote:
> > > I have some concern about this call back to the original AsmPrinter::emitLinkage.
> > > It seems this is just a short cut so that we don't need to handle the linkage emission with `unspecified` visibility.
> > > But I would argue that taking this short cut might not worth it, as it's not too much work for the emitXCOFFSymbolLinkageWithVisibility to handle the `unspecified` visibility case.
> > > If we don't need to call back to the base AsmPrinter::emitLinkage, then we could remove a lot of XCOFF-related staff out from AsmPrinter::emitLinkage which would make the code a lot cleaner. Also we could avoid dual-maintaining two emitLinkage method for AIX.
> > some linkage do not have visibility.  
> > for example     
> > InternalLinkage .lglobl , it should go to PPCAsmPrinter::emiltLinkage.
> > 
> > and there also have PrivateLinkage and AppendingLinkage, we do not deal with in the function , it just need to pass to function PPCAsmPrinter::emiltLinkage to deal with it.
> > 
> > I discuss with jason offline, Jason's option is "we should deal with all the linkage and visibility in current function without invoke the PPCAsmPrinter::emiltLinkage()" . 
> > @hubert.reinterpretcast , what is your option ?
> Thanks for the change. I do like what we have right now because we don't need to worry if the base emitLinkage have the correct implementation for AIX or not.
> Are we planning to clean up the base emitLinkage (remove XCOFF-related changes) in a separate NFC patch?
Only one place need to clean in the base emitLinkage, I clean it in this patch.


================
Comment at: llvm/test/CodeGen/PowerPC/aix-xcoff-hidden.ll:47
+; CHECK:        .globl  .foo_h,hidden
+; CHECK:        .globl  foo_protected[DS],protected
+; CHECK:        .globl  .foo_protected,protected
----------------
hubert.reinterpretcast wrote:
> The file is called `aix-xcoff-hidden.ll`? Should it be renamed?
thanks


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75866