[PATCH] D70718: [AIX] Replace ExternalSymbolSDNode with a MCSymbolSDNode for memcpy/memset/memmove

[Hubert Tong via Phabricator via llvm-commits]

hubert.reinterpretcast added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:5091
+      .Cases("__moddi3", "__divdi3", "__umoddi3", "__udivdi3", true)
+      .Cases("__floatdidf", "__fixdfdi", "__fixunsdfdi", "__floatdisf",
+             "__floatundidf", "__floatundisf", true)
----------------
Is there a rationale to the ordering here? I don't see a semantic or lexical reason for the current ordering. Ordering helps when comparing lists.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:5114
 
+  const auto replaceCalleeWithAIXFuncEntryPoint =
+      [&](StringRef FuncName, bool IsDeclaration,
----------------
The name of this reads like an operation. It is not clear what the return value is (but it does return a value). Is this more `getAIXFuncEntryPointSymbol`?


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:5133
+        // Replace the callee SDNode with the MCSymbolSDNode.
+        EVT PtrVT =
+            DAG.getTargetLoweringInfo().getPointerTy(DAG.getDataLayout());
----------------
I think this should be an `MVT` to match the return type.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:5149
     // C-linkage name.
-    auto &Context = DAG.getMachineFunction().getMMI().getContext();
-
-    const GlobalObject *GO = cast<GlobalObject>(G->getGlobal());
-    MCSymbolXCOFF *S = cast<MCSymbolXCOFF>(
-        Context.getOrCreateSymbol(Twine(".") + Twine(GO->getName())));
-
-    if (GO && GO->isDeclaration() && !S->hasContainingCsect()) {
-      // On AIX, an undefined symbol needs to be associated with a
-      // MCSectionXCOFF to get the correct storage mapping class.
-      // In this case, XCOFF::XMC_PR.
-      const XCOFF::StorageClass SC =
-          TargetLoweringObjectFileXCOFF::getStorageClassForGlobal(GO);
-      MCSectionXCOFF *Sec =
-          Context.getXCOFFSection(S->getName(), XCOFF::XMC_PR, XCOFF::XTY_ER,
-                                  SC, SectionKind::getMetadata());
-      S->setContainingCsect(Sec);
+    assert(GO &&
+           "The GlobalAddressSDNode does not have a global object associated.");
----------------
The assert is unnecessary. `cast` does not return null pointers.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:5164
+    assert(SymName &&
+           "The ExternalSymbolSDNode does not have a MCSymbol associated.");
+    // TODO: Remove this when the support for ExternalSymbolSDNode is complete.
----------------
Do you mean that there is no name?


================
Comment at: llvm/test/CodeGen/PowerPC/aix-external-sym-sdnode-lowering.ll:40
+; 32BIT: BL_NOP <mcsymbol .memset>
+; 64BIT: BL8_NOP <mcsymbol .memset>
----------------
The test does not cover the larger set of strings that you whitelisted. I'm not sure if that is intentional.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70718