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

[Xiangling Liao via Phabricator via llvm-commits]

Xiangling_L marked 5 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:
> 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`?
I see what you are talking about here. I refactored the code and will update the patch soon.


================
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:
> 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`.
It seems also belong to the follow-up performance patch. But I am also curious that any suggestion of it? @sfertile 


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:5367
+              S->getName(), XCOFF::XMC_PR, XCOFF::XTY_ER, XCOFF::C_EXT,
+              SectionKind::getMetadata());
+          S->setContainingCsect(Sec);
----------------
@hubert.reinterpretcast  @cebowleratibm @sfertile @jasonliu Since we didn't get the chance to talk about this in the scrum, I am thinking I can put some of my thoughts here for discussion.

1. I would treat GCC and LLVM 's behavior as a bug in this case. The compiler shouldn't synthesize to a `memcpy` which conflicts with user-defined `memcpy` and lead to some undefined behavior. The correct behavior seems should be when the compiler detect a user-defined/user-declared `memcpy`, it should give up the synthesis and keep original `load` and `store`.

Difficulties: We need a way to tell if there is a user-declared/user-defined `memcpy` in the translation unit when the compiler make the decision that if we want to replace a bunch of `load` and `store` with `memcpy` ExternalSymbolSDNode.

2. On the other hand, if we want to follow GCC's behavior and try to pick up user-defined/user-declared `memcpy` to replace original bunch of `load` and `store`, we also need to way to tell if there is a user-declared/user-defined `memcpy` in the translation unit, based on which we set `Csect` accordingly.

In summary, it seems a way to tell if there is a user-declared/user-defined `memcpy` in the translation unit is a must, i.e. if there is a way to tell if there exists a `memcpy` GlobalObject. But so far I got no luck to figure out how to do that.  Anyone has any thoughts about that?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70718