[PATCH] D78045: [XCOFF][AIX] Fix getSymbol to return the correct qualname when necessary

[Jason Liu via Phabricator via llvm-commits]

jasonliu added inline comments.


================
Comment at: llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp:1900
+          ->getQualNameSymbol();
+    if (GOKind.isCommon() || GOKind.isBSSLocal())
+      return cast<MCSectionXCOFF>(SectionForGlobal(GO, GOKind, TM))
----------------
DiggerLin wrote:
> add some thing like to test isBSSLocal by the way ?
> 
> @comm_bs = internal global i32 0, align 4
> @ps = global i32* @comm_bs, align 4
I don't feel a strong need about this, as this code path is going to get exercise by emitGlobalVariable and  getMCSymbolForTOCPseudoMO, and so we know we will get a qualname for it and is covered by the old test cases already.


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1614
       OutStreamer->emitXCOFFLocalCommonSymbol(
-          GVSym, Size, Csect->getQualNameSymbol(), Align);
+          OutContext.getOrCreateSymbol(GVSym->getUnqualifiedName()), Size,
+          GVSym, Align);
----------------
hubert.reinterpretcast wrote:
> DiggerLin wrote:
> > since we have a qualname symbol, I do not think we need to create and unnecessary  UnqualifiedName symbol.
> > maybe we change change the interface of emitXCOFFLocalCommonSymbol. I do not think we need to pass two Symbols(MCSymbol *LabelSym,  and  MCSymbol *CsectSym) in the emitXCOFFLocalCommonSymbol.
> The label does not have to be given linkage, but it is not optional in the assembly syntax.
`.lcomm  a[BS],4,a[BS],2` does not work. First expression must be a label.  


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1603
   // since this may not have been set elsewhere depending on how they are used.
   MCSectionXCOFF *Csect = cast<MCSectionXCOFF>(
       GV->isDeclaration()
----------------
DiggerLin wrote:
> since we have created the MCSectionXCOFF *Csect in the getSymbol.
> do we still need this ?
> 
> can we change to  
> if (GVSym->hasRepresentedCsectSet())
>   Csect = GVSym->getRepresentedCsect()
> else {
>   Csect = getObjFileLowering().SectionForGlobal(
>           : getObjFileLowering().SectionForGlobal(
>                 GV, GVKind = getObjFileLowering().getKindForGlobal(GV, TM),
> }
> ?
>   
Thanks. I don't think we need to get csect for external symbol. For the other symbol, even if it's qualname, we could still get it with SectionForGlobal query.
I would like to avoid the usage of hasRepresentedCsectSet() if possible. Whenever we use that interface means we have a potential timing issue (some are indeed by design) for csect setting. 
So I adjust the code accordingly.


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

https://reviews.llvm.org/D78045