[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:5353
+
+    if (ExternalSymbolSDNode *E = dyn_cast<ExternalSymbolSDNode>(Callee)) {
+      const char *SymName = E->getSymbol();
----------------
Xiangling_L wrote:
> hubert.reinterpretcast wrote:
> > If the two conditions are mutually exclusive, then this should be an `else if`. If this check is meant to operate on the new `Callee` assigned in the block above, then a comment really should be added.
> Thanks for mentioning that, but in LLVM coding standard:  [[ https://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return | Don’t use else after a return ]],  so I didn't use `else if ` here.
Exactly how does that part of the coding standard apply here? This `if` is reachable after the previous `if` block has been entered. Again, the previous `if` block modifies `Callee`. Is this check meant to operate upon the modified `Callee`?


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:5356
+      //TODO: Remove this when the support ExternalSymbolSDNode is complete.
+      if (strcmp(SymName, "memcpy") == 0 || strcmp(SymName, "memmove") == 0 ||
+          strcmp(SymName, "memset") == 0) {
----------------
Xiangling_L wrote:
> hubert.reinterpretcast wrote:
> > sfertile wrote:
> > > I think there will be a handful of other symbols we will need to white list for now as well (not needed in this patch, but sooner rather then later). I suggest we add a function that determines if the name an ExternalSDNode refers to is one we support by using a StringSwitch of the accepted names. When a name is not a supported/expected symbol then we can dump the SDNode as part of the debug tracing, and fatal error.
> > There are other libc extended operations that don't seem to come through as an ExternalSDNode by default, such as for `memcmp`. @sfertile, do you have an suggested approach for this?
> I am not sure if I understand what you said correctly, but I think for those SDNode which doesn't come through as an `ExternalSDNode` will fall into `GlobalAddressSDNode`, for which we already add "." as a prefix for function entry point in previous commit?
> 
> If I am wrong, can you state your concern more clearly?
This has to do with mapping a call to `memcmp` to the entry point symbol named `.___memcmp`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70718