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

[Hubert Tong via Phabricator via llvm-commits]

hubert.reinterpretcast added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/TargetLoweringObjectFileImpl.h:250
+  MCSection *
+  getSectionForExternalReference(const GlobalObject *GO,
+                                 const TargetMachine &TM) const override;
----------------
daltenty wrote:
> hubert.reinterpretcast wrote:
> > I am not sure there is anything about this interface that indicates that it is not suitable for retrieving the section for an external reference to a function entry point. Where `getAIXFuncEntryPointSymbolSDNode` is present in the codebase, the function is also handled as a `GlobalObject`.
> Indeed, that is not a limitation of this interface, this is the case in PPCISelLowering mentioned in the patch description. It was my intention to handle that as a separate patch since it will require some further refactoring as it appears the TLOF is not available there. 
The interface is ambiguous as it stands on whether the function descriptor or function entry point is meant.


================
Comment at: llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp:1970
+    const MCSymbol *FuncSym) const {
+  return getContext().getXCOFFSection(FuncSym->getName(), XCOFF::XMC_DS,
+                                      XCOFF::XTY_SD, XCOFF::C_HIDEXT,
----------------
There should be an assertion that the function is defined.


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1806
+    return;
+
   if ((!GVKind.isCommon() && !GVKind.isBSS() && !GVKind.isData() &&
----------------
daltenty wrote:
> hubert.reinterpretcast wrote:
> > Assert `!GV->isDeclaration()` here to be sure that `GVKind` has been initialized.
> The branch on `!GV->hasInitializer()` above guarantees this, `GV->hasInitializer()` is defined as `!isDeclaration()`
It is better not to rely on such a relationship between the two queries.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72347