[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:5182
 
-    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.
+    // If there exists user-defined function whose name is the same as the
+    // ExternalSymbol's, we pick up user-defined version.
----------------
Add "a" before "user-defined".


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:5182
 
-    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.
+    // If there exists user-defined function whose name is the same as the
+    // ExternalSymbol's, we pick up user-defined version.
----------------
hubert.reinterpretcast wrote:
> Add "a" before "user-defined".
Both instances of "user-defined" should be "user-declared".


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:5183
+    // If there exists user-defined function whose name is the same as the
+    // ExternalSymbol's, we pick up user-defined version.
+    const Module *Mod = DAG.getMachineFunction().getFunction().getParent();
----------------
Add "the" before "user-defined".


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:5186
+    if (const GlobalValue *GV = Mod->getNamedValue(SymName)) {
+      const GlobalObject *GO = dyn_cast<GlobalObject>(GV);
       const XCOFF::StorageClass SC =
----------------
`dyn_cast` can return a null pointer.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:5190
+      return getAIXFuncEntryPointSymbol(GO->getName(), GO->isDeclaration(), SC);
+    } else {
+      // TODO: Remove this when the support for ExternalSymbolSDNode is complete.
----------------
This code does not need to be inside an `else` block.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:5195
+      } else {
+        report_fatal_error("Unexpected ExternalSymbolSDNode: " + Twine(SymName));
+      }
----------------
This does not need to be inside an `else` block.


================
Comment at: llvm/test/CodeGen/PowerPC/aix-user-defined-memcpy.ll:20
+
+; CHECK: bl .memcpy
+; CHECK-NOT: error
----------------
This test should preferably check the symbol table for a `.o` file and the relocation associated with the call.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70718