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

[Xiangling Liao via Phabricator via llvm-commits]

Xiangling_L marked 14 inline comments as done.
Xiangling_L added inline comments.


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


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


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:5366
+          MCSectionXCOFF *Sec = Context.getXCOFFSection(
+              S->getName(), XCOFF::XMC_PR, XCOFF::XTY_ER, XCOFF::C_EXT,
+              SectionKind::getMetadata());
----------------
hubert.reinterpretcast wrote:
> What happens if the user is defining `memset` in this file?
I did some test, and I think it's worth bringing up the findings in the srum, but I will document what I found here.


```
[test_user_define_memcpy.c]
int memcpy ( void * destination, int num ) { return 3; }

typedef struct cpy_array {
  int array[20];
} cpy;

int main() {
  cpy A = {1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15,16};
  cpy B = A;
  return B.array[0];
}

```
1. XLC on AIX with no-opt:  
XLC doesn't synthesize load & store to `memcpy`, the result is correct `=1`.

2. GCC on AIX with no-opt:
GCC in this case didn't pick up library version `memcpy`, and the result was some random garbage value.

3. LLVM with `powerpc64le-unknown-linux-gnu` target with no-opt:
the same as above GCC case.

So I think the following questions are:
1) Is GCC and LLVM's behavior in this case a bug?
2) Do we want to follow GCC's behavior?
3) If the answer of `2)` is yes, how do we want to achieve that, especially when the definition of `memcpy` is put after `callInst` of `ExternalSymbolSDNode`, like the following:


```
typedef struct cpy_array {
  int array[20];
} cpy;

int main() {
  cpy A = {1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15,16};
  cpy B = A;
  return B.array[0];
}

int memcpy ( void * destination, int num ) { return 3; }

```


================
Comment at: llvm/test/CodeGen/PowerPC/aix-external-sym-sdnode-lowering.ll:11
+entry:
+  call void @llvm.memcpy.p0i8.p0i8.i32(i8* %p, i8* %q, i32 %n, i1 false)
+  ret void
----------------
hubert.reinterpretcast wrote:
> XL calls `.___memmove` (with three underscores) with no NOP to implement both `memcpy` and `memmove`. This is implemented in libc as an extended operation (with no TOC dependency). This is also the case with `.___memset` for `memset`.
Thank you for bringing this up, we discussed it in the scrum today, and it's preferred to defer this to a performance related change when we identify more similar cases.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70718