[PATCH] D72347: [NFC][XCOFF] Refactor Csect creation into TargetLoweringObjectFile

[David Tenty via Phabricator via llvm-commits]

daltenty marked 3 inline comments as done.
daltenty added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/TargetLoweringObjectFileImpl.h:251
+  MCSection *
+  getSectionForExternalReference(const GlobalObject *GO,
+                                 const TargetMachine &TM) const override;
----------------
DiggerLin wrote:
> I think we can 
>  getSectionForExternalReference(const MCSymbol *Sym). const override  here ?
> 
> I search all the place where invoke getSectionForExternalReference , it can use the function define  getSectionForExternalReference(const MCSymbol *Sym)
> 
> the benefit of using  getSectionForExternalReference(const MCSymbol *Sym) , we save one parameter and have some function parameter as getSectionForFunctionDescriptor  and getSectionForTOCEntry
I was original inclined to do this as well, but we need to query the GlobalObject to determine whether it is a function, so we can set the correct Storage Mapping Class.


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1803
+  // External global variables are already handled.
+  if (!GV->hasInitializer())
+    return;
----------------
DiggerLin wrote:
> If I understand correctly, I do not think we need to change the 
>   if (!GV->hasInitializer())  to here .
> 
> the reason is  if the extern variable is declared in the source code. but never be used, it the declare extern variable do not to show on the xcoff object file even in the symbol table or in the asm file.
> 
> if the extern variable is  declared and be used as  initiator , the MCSection will be created in the PPCAIXAsmPrinter::lowerConstant() 
There are other cases than will be handled by lowerConstant(). We may refer to the external global in a function body, in which case we will need to setup the symbols containing CSect here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72347