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

[Xiangling Liao via Phabricator via llvm-commits]

Xiangling_L added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:5322
 
-  if (Subtarget.isAIXABI() && isFunctionGlobalAddress(Callee)) {
+  if (Subtarget.isAIXABI()) {
     // On AIX, direct function calls reference the symbol for the function's
----------------
jasonliu wrote:
> Are we assuming when entering here, Callee could only be "isFunctionGlobalAddress" and "ExternalSymbolSDNode"?
> If so, an assertion here would be necessary. If not, we would end up replace the callee with a (MCSymbolXCOFF)nullptr, which would be wrong. 
Good point. The Callee entering here won't only be `isFunctionGlobalAddress` and `ExternalSymbolSDNode`.




================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:5345
+
+    if (ExternalSymbolSDNode *E = dyn_cast<ExternalSymbolSDNode>(Callee)) {
+      const char *SymName = E->getSymbol();
----------------
jasonliu wrote:
> The two if statement inside of "Subtarget.isAIXABI()" have very similar content. It would be great if we could do something to common things up.
I would prefer to make it cleaner as well, but when I take a closer look, though each part goes through similar process, each step is kinda different. Especially, for external symbol sdnode, we only cover three cases so far. All those make it a little bit tricky to common things up. I would appreciate that if you give some more detailed thoughts about how to common things up here.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:5347
+      const char *SymName = E->getSymbol();
+      if (strcmp(SymName, "memcpy") == 0 || strcmp(SymName, "memmove") == 0 ||
+          strcmp(SymName, "memset") == 0) {
----------------
jasonliu wrote:
> I still kinda prefer this to be in an assert to begin with. 
> But if we decide that we want this expensive pattern matching to run in non-assert build as well, then we could keep this way. 
> Is this some code that we are going to remove later when we think we covered all the cases? If so, do we want to leave a TODO here?
I can leave this open for a while to get more inputs and I will add a `TODO` as you suggested. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70718