[PATCH] D70461: [AIX] Emit TOC entries for ASM printing

[Jason Liu via Phabricator via llvm-commits]

jasonliu added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1910
+
+  return PPCAsmPrinter::getMCSymbolForTOCPseudoMO(MO);
+}
----------------
daltenty wrote:
> jasonliu wrote:
> > Could we use this structure for the function? I think it's easier to read and understand. 
> > 
> > ```
> > if (MO.getType() != MachineOperand::MO_GlobalAddress)
> >   return PPCAsmPrinter::getMCSymbolForTOCPseudoMO(MO);
> > 
> > const GlobalVariable *GV = MO.getGlobal();
> > ValidateGV(GV)
> > if (GV->isDeclaration()) {
> >   if (!Xsym->hasContainingCsect()) {
> >     // Set up csect for external symbol, use isa<Function> to figure out what SMC we should use for the csect. 
> >   }
> >   return Csect->getQualNameSymbol();
> > }
> > 
> > SectionKind GVKind = getObjFileLowering().getKindForGlobal(GV, TM);
> > if (GVKind.isCommon() || GVKind.isBSSLocal()) {
> >   return Csect->getQualNameSymbol();
> > }
> > 
> > return getSymbol(GV);
> > ```
> > 
> > Some remark on some implication on this change:
> > 1.
> > ```
> >  !(GO = dyn_cast<const GlobalObject>(MO.getGlobal())  
> > ```
> >  I'm not sure why we need this dyn_cast on the first if, so I removed it.
> > 
> > 2. 
> > 
> > ```
> > const GlobalVariable *GV = dyn_cast<const GlobalVariable>(GO);
> >   if (GV) {
> >     ValidateGV(GV);
> > ```
> > This is a upcast, it's definitely going to succeed. So we don't need dyn_cast, and we will get a GV for sure. 
> > 
> > 3. On your current function, it will return a qual name for a defined function. And this structure will not return a qual name for the defined function.
> >   I think the qual name for the defined function is unnecessary, and use label to refer to that function will succeed. And it's easier on the mental model for when we should use a qual name for TC entry: when the symbol is undefined, or when it's common. 
> I have a few issues with the specifics of this, as outlined bellow, but I think I can generally adopt this direction. I agree some of the GlobalVariable && Function case can just be folded so I'll try to follow that.
> 
> First on the remarks:
> 
> 1. MO.getGlobal() returns a GlobalValue, but we only want to handle GlobalObject cases (i.e. GlobalVariable & Function), so some kind of cast is required
> 
> 2. This is not an upcast, GO is a GlobalObject.
> 
> 3. Hmm, I actually found it easier to reason about referring to the CSects directly when appropriate, which is why I included that case. 
> 
> Essentially, it boils down to "If you're talking about something that is a whole CSect in it's own right (i.e. Function Descriptors), refer to by it's QualNameSymbol".  Why refer to all these extra middle-men label symbols when they aren't needed? 
> 
> That said, if we prefer to keep this minimal and use the existing label, I can add in the checks to pass it by.
> 1. MO.getGlobal() returns a GlobalValue, but we only want to handle GlobalObject cases (i.e. GlobalVariable & Function), so some kind of cast is required
I see. I think the behavior is still correct because it would fall back to the last "return getSymbol(GV);", which is what PPCAsmPrinter::getMCSymbolForTOCPseudoMO(MO) do anyway. But I agree we could just do the early return.

> 2. This is not an upcast, GO is a GlobalObject.
You are right. I think I mis-read the GV as GlobalValue. (I need to be careful when I see "GV" because it could mean GlobalValue or GlobalVariable.)

> 3. Essentially, it boils down to "If you're talking about something that is a whole CSect in it's own right (i.e. Function Descriptors), refer to by it's QualNameSymbol". 
Okk, I'm Okay with the mental model as "If we could refer to it as a csect, then let's refer to it as a csect.". 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70461