[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/MC/MCXCOFFStreamer.cpp:57
+
+  ///< Not emit default visibility.
+  if (Visibility == MCSA_Invalid)
----------------
DiggerLin wrote:
> jasonliu wrote:
> > jasonliu wrote:
> > > hubert.reinterpretcast wrote:
> > > > The comment here should not be Doxygen style. There should be a comment elsewhere (in the header for the base class declaration) that //is// in Doxygen style. Also, "default visibility" is ambiguous for XCOFF.
> > > > 
> > > > Suggestion:
> > > > ```
> > > >   // When the caller passes `MCSA_Invalid` for the visibility, do not emit one.
> > > > ```
> > > > There should be a comment elsewhere (in the header for the base class declaration) that is in Doxygen style.
> > > I'm not seeing this comment being addressed?
> > I don't think removing 
> > ```
> > if (Visibility == MCSA_Invalid)
> >     return;
> > ```
> > before emit symbol attribute for visibility, and skip the requested comment is a good way to move forward.
> > Without those, there is no clear contract to tell people how to use this function. 
> if Visibility is  MCSA_Invalid, it will not come to function emitXCOFFSymbolLinkageWithVisibility
Thanks. I would actually want the emitXCOFFSymbolLinkageWithVisibility to handle MCSA_Invalid case. Please see my other comment. 


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1587
+  case GlobalValue::DefaultVisibility:
+    PPCAsmPrinter::emitLinkage(GV, GVSym);
+    return;
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75866