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

[Jason Liu via Phabricator via llvm-commits]

jasonliu added inline comments.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:424
   case GlobalValue::ExternalLinkage:
     if (MAI->hasDotExternDirective() && GV->isDeclaration())
       OutStreamer->emitSymbolAttribute(GVSym, MCSA_Extern);
----------------
hasDotExternDirective is XCOFF only. 
Also, please check if we have other usage of this MAI property. If not, we could remove them together.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:432
   case GlobalValue::InternalLinkage:
     if (MAI->hasDotLGloblDirective())
       OutStreamer->emitSymbolAttribute(GVSym, MCSA_LGlobal);
----------------
XCOFF only. Clean up needed. Please also check if we could remove this property from MAI.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:436
   case GlobalValue::ExternalWeakLinkage:
     if (TM.getTargetTriple().isOSBinFormatXCOFF()) {
       OutStreamer->emitSymbolAttribute(GVSym, MCSA_Weak);
----------------
This is XCOFF only, which could get clean up. 


================
Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:442
   case GlobalValue::AvailableExternallyLinkage:
     if (MAI->hasDotExternDirective()) {
       OutStreamer->emitSymbolAttribute(GVSym, MCSA_Extern);
----------------
XCOFF only. Clean up needed.


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1587
+  case GlobalValue::DefaultVisibility:
+    PPCAsmPrinter::emitLinkage(GV, GVSym);
+    return;
----------------
DiggerLin wrote:
> 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.
There is more than one place. Please check my other comments if we want to clean it in this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75866