[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:5122
+      .Cases("__divdi3", "__fixdfdi", "__fixunsdfdi", "__floatdidf",
+             "__floatdisf", "__floatundidf", "__floatundisf", "__moddi3",
+             "__udivdi3", "__umoddi3", true)
----------------
There is no demonstration that we should expect to encounter `__floatdisf`. Should we retain it in this list? Similarly for other cases not present in the test.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:5155
+  // C-linkage name.
+  const auto getAIXFuncEntryPointSymbol =
+      [&](StringRef FuncName, bool IsDeclaration,
----------------
Sorry for missing this earlier. Would it be better to name this `getAIXFuncEntryPointSymbolSDNode`?


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:5173
+
+        // Replace the callee SDNode with the MCSymbolSDNode.
+        MVT PtrVT =
----------------
Remove this comment (it no longer fits here).


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:5206
+        return getAIXFuncEntryPointSymbol(F->getName(), F->isDeclaration(), SC);
+      }
+
----------------
I find it very jarring when the next statement after a closing brace is not at the same indentation level (and similarly when the last statement before a closing brace is not indented just one level in).


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:5209
+    // TODO: Remove this when the support for ExternalSymbolSDNode is complete.
+    if (isValidAIXExternalSymSDNode(SymName)) {
+      return getAIXFuncEntryPointSymbol(SymName, true, XCOFF::C_EXT);
----------------
This `if` does not need braces. I would much rather spend the braces on the `if` statement above.


================
Comment at: llvm/test/CodeGen/PowerPC/aix-user-defined-memcpy.ll:55
+
+; 32-REL: LLVM ERROR: Unexpanded relocation output not implemented.
+
----------------
Use `--expand-relocs` to get past this message.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70718