[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:5367
+              S->getName(), XCOFF::XMC_PR, XCOFF::XTY_ER, XCOFF::C_EXT,
+              SectionKind::getMetadata());
+          S->setContainingCsect(Sec);
----------------
Xiangling_L wrote:
> hubert.reinterpretcast wrote:
> > Xiangling_L wrote:
> > > @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?
> > The LLVM and GCC behaviour is acceptable. Under C90 subclause 7.1.3, a user definition of `memcpy` leads to undefined behaviour. At the assembly and object generation level, we can handle cases where the user leaves `memcpy` as a function in a somewhat reasonable manner. If the user defines `memcpy` as an object, then we will preferably have a way to keep both `memcpy` symbols around even if the resulting assembly or object file is not exactly valid.
> > 
> > As for finding out whether the user has defined `memcpy`, I think we can look into `llvm::Module::getNamedValue`.
> Do you mean we don't want to either follow LLVM and GCC's behavior as `Under C90 subclause 7.1.3, a user definition of memcpy leads to undefined behavior` or not synthesize `memcpy` but introduce a third way to handle this by generating invalid assembly or object files? Can you state a little bit clearly why we want to do this rather than following LLVM and GCC? And how does `invalid assmbly or object file` will possibly look like if take `memcpy` for example?
It is okay to synthesize calls to `memcpy` expecting that the user did not define it themselves; however, to encode such calls when the symbol is defined in the current translation unit as a variable could be problematic (although less so if the symbols are clearly distinguished, and with the final XCOFF, the object and the function entry point would be distinctly named). As for your question regarding why we are not following LLVM and GCC, I have not said suggested anything that contradicts the LLVM and GCC behaviour as you have described it. The implication in what I said is that the goal is to not trigger an internal compiler error. It seems the scope for which we need to be concerned is already limited by Clang:
```
error: redefinition of 'memcpy' as different kind of symbol
```

I guess a problematic case could still be presented through LLVM IR.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70718