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

[Xiangling Liao via Phabricator via llvm-commits]

Xiangling_L marked 11 inline comments as done.
Xiangling_L 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);
----------------
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?


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:5091
+      .Cases("__moddi3", "__divdi3", "__umoddi3", "__udivdi3", true)
+      .Cases("__floatdidf", "__fixdfdi", "__fixunsdfdi", "__floatdisf",
+             "__floatundidf", "__floatundisf", true)
----------------
hubert.reinterpretcast wrote:
> Is there a rationale to the ordering here? I don't see a semantic or lexical reason for the current ordering. Ordering helps when comparing lists.
Thanks, I will order them alphabetically.


================
Comment at: llvm/test/CodeGen/PowerPC/aix-external-sym-sdnode-lowering.ll:40
+; 32BIT: BL_NOP <mcsymbol .memset>
+; 64BIT: BL8_NOP <mcsymbol .memset>
----------------
hubert.reinterpretcast wrote:
> The test does not cover the larger set of strings that you whitelisted. I'm not sure if that is intentional.
Thanks for mentioning that, I was planning to update the testcase/add more testcases when the above `ExternalSymbolSDNode undefined behavior` discussion is closed. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70718