[PATCH] D80642: [AIX] Emit AvailableExternally Linkage on AIX

[Digger via Phabricator via llvm-commits]

DiggerLin added inline comments.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:442
   case GlobalValue::AvailableExternallyLinkage:
+    if (TM.getTargetTriple().isOSBinFormatXCOFF()) {
+      OutStreamer->emitSymbolAttribute(GVSym, MCSA_Extern);
----------------
Xiangling_L wrote:
> DiggerLin wrote:
> > Xiangling_L wrote:
> > > DiggerLin wrote:
> > > > DiggerLin wrote:
> > > > > I think we look AvailableExternallyLinkage as AvailableExternallyLinkage
> > > > > 
> > > > > you can change as  
> > > > > ```
> > > > > case GlobalValue::ExternalLinkage:
> > > > > case GlobalValue::AvailableExternallyLinkage:
> > > > >     if (MAI->hasDotExternDirective() && GV->isDeclarationForLinker()) {
> > > > > }
> > > > > ```
> > > > > and we do not need code 
> > > > > ````
> > > > > case GlobalValue::AvailableExternallyLinkage:
> > > > >     if (TM.getTargetTriple().isOSBinFormatXCOFF()) {
> > > > >    ....
> > > > > }
> > > > > ````
> > > > > here.
> > > > sorry it should be "I think we look AvailableExternallyLinkage as ExternalLinkage"
> > > Thank you for your suggestion. I agree that it would give the query  more semantic meaning. But since if a GV falls under the `AvailableExternallyLinkage` category, then `GV->isDeclarationForLinker()`is always true. So it seems we only need `MAI->hasDotExternDirective()` here?
> > but for ExternalLinkage, it need GV->isDeclarationForLinker(). otherwise it will emit .globl symbolName.
> Sorry, I don't make it clear. But I don't think we should combine `ExternalLinkage` and `AvailableExternallyLinkage`. Because for other targets, we still want to fall throught to `llvm_unreachable`.
o, yes, you are correct, for other target, we still want to fa ll throught to llvm_unreachable.  we maybe to change 
if (TM.getTargetTriple().isOSBinFormatXCOFF()) to 
 if (MAI->hasDotExternDirective())



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80642