[PATCH] D75866: [AIX] supporting the visibility attribute for aix assembly
Digger via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Jun 5 08:18:46 PDT 2020
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
More information about the llvm-commits
mailing list