[PATCH] D36831: [AMDGPU] Transform __read_pipe_* and __write_pipe_*

[Yaxun Liu via Phabricator via llvm-commits]

yaxunl marked an inline comment as done.
yaxunl added inline comments.


================
Comment at: lib/Target/AMDGPU/AMDGPULibCalls.cpp:616
+
+  auto *F = M->getOrInsertFunction(Name, FTy);
+  auto *NCI = B.CreateCall(F, Args);
----------------
rampitec wrote:
> rampitec wrote:
> > yaxunl wrote:
> > > rampitec wrote:
> > > > You should not use getOrInsertFunction directly, but use AMDGPULibCalls::getFunction().
> > > > Since you are not using it you also fail to check we are in pre-link.
> > > whether in pre-link can be checked by whether the function is declaration.
> > > 
> > > `__read_pipe_*` and `__write_pipe_*` are unmangled functions. AMDGPULibFunc::getFunction relies on argument information encoded in mangled function names, which does not work for unmangled functions.
> > You are still using M->getOrInsertFunction() directly.
> I see you are now checking for the declaration vs definition, but it is still only legal on pre-link and that is not checked.
Now use AMDGPULibFunc::getOrInsertFunction().


================
Comment at: lib/Target/AMDGPU/AMDGPULibFunc.h:366
 
-  void          reset();
+  static StringRef getUnmangledName(const StringRef &mangledName);
 
----------------
vpykhtin wrote:
> rampitec wrote:
> > yaxunl wrote:
> > > rampitec wrote:
> > > > StringRef designed to be passed by value, here and below.
> > > This comes from the original implementation. Will fix.
> > Not done.
> Initially (non-const) StringRef& was introduced intentionally to have mangledName with stripped name on return.
Only const StringRef& is changed to StringRef. non-const StringRef& is not changed.


https://reviews.llvm.org/D36831