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

[Jason Liu via Phabricator via llvm-commits]

jasonliu accepted this revision.
jasonliu added a comment.
This revision is now accepted and ready to land.

LGTM with minor nit.



================
Comment at: llvm/include/llvm/MC/MCStreamer.h:572
+  /// \param Linkage - The linkage of the symbol to emit.
+  /// \param Visibility - The visibility of the symbol to emit. 
+  virtual void emitXCOFFSymbolLinkageWithVisibility(MCSymbol *Symbol,
----------------
nit, we need to mention it here as Doxygen style:
When the symbol do not have any visibility setting, pass in `MCSA_Invalid`.


================
Comment at: llvm/lib/MC/MCAsmStreamer.cpp:818
+  switch (Visibility) {
+  default:
+    report_fatal_error("unexpected value for Visibility");
----------------
nit: we should make the default case position more consistent at least within this function/file.


================
Comment at: llvm/lib/MC/MCAsmStreamer.cpp:819
+  default:
+    report_fatal_error("unexpected value for Visibility");
+  case MCSA_Invalid:
----------------
nit: Visibility -> visibility type


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75866