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

[Jason Liu via Phabricator via llvm-commits]

jasonliu added inline comments.
Herald added a subscriber: wuzish.


================
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
----------------
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. 


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:5345
+
+    if (ExternalSymbolSDNode *E = dyn_cast<ExternalSymbolSDNode>(Callee)) {
+      const char *SymName = E->getSymbol();
----------------
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.


================
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) {
----------------
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?


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:5351
+
+	S = cast<MCSymbolXCOFF>(
+            Context.getOrCreateSymbol(Twine(".") + Twine(SymName)));
----------------
nit: formatting off here.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:5356
+          // get the correct storage mapping class. In this case, XCOFF::XMC_PR.
+	  MCSectionXCOFF *Sec = Context.getXCOFFSection(
+              S->getName(), XCOFF::XMC_PR, XCOFF::XTY_ER, XCOFF::C_EXT,
----------------
nit: formatting off here. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70718