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

[David Tenty via Phabricator via llvm-commits]

daltenty marked an inline comment as done.
daltenty added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1910
+
+  return PPCAsmPrinter::getMCSymbolForTOCPseudoMO(MO);
+}
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70461