[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:250
+  MCSection *
+  getSectionForExternalReference(const GlobalObject *GO,
+                                 const TargetMachine &TM) const override;
----------------
hubert.reinterpretcast wrote:
> 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.
I've added a comment to clarify this is the behaviour for XCOFF


================
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,
----------------
hubert.reinterpretcast wrote:
> There should be an assertion that the function is defined.
That query is not safe to make on the symbol here, as it requires a fragment that we may not have. 


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1806
+    return;
+
   if ((!GVKind.isCommon() && !GVKind.isBSS() && !GVKind.isData() &&
----------------
hubert.reinterpretcast wrote:
> 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.
I will replace the branch condition above with 'isDeclaration()' so the relationship is explicit


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72347