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

[Digger via Phabricator via llvm-commits]

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


================
Comment at: llvm/lib/MC/MCXCOFFStreamer.cpp:57
+
+  ///< Not emit default visibility.
+  if (Visibility == MCSA_Invalid)
----------------
jasonliu wrote:
> 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. 
please see my other comment.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75866