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

[Yaxun Liu via Phabricator via llvm-commits]

yaxunl marked 8 inline comments as done.
yaxunl added inline comments.


================
Comment at: lib/Target/AMDGPU/AMDGPULibCalls.cpp:67
   typedef llvm::AMDGPULibFunc FuncInfo;
+  typedef llvm::AMDGPUMangledLibFunc MangledFuncInfo;
+  typedef llvm::AMDGPUUnmangledLibFunc UnmangledFuncInfo;
----------------
rampitec wrote:
> It is better hide these details from the client. AMDGPULibFunc should care about all of it and FuncInfo shall be sufficient structure to describe a function. Simplifier should not care about details.
The handling of mangled lib function requires function argument information, which is not available for unmangled lib function.

The transformation of unmangled lib function does not require function argument information. There is no point of implementing many of the member functions of mangled lib functions.


================
Comment at: lib/Target/AMDGPU/AMDGPULibFunc.cpp:987
+bool UnmangledFuncInfo::lookup(StringRef Name, ID &Id) {
+  for (unsigned I = 0; I != TableSize; ++I) {
+    if (Name == Table[I].Name) {
----------------
rampitec wrote:
> I believe we can go without bruteforce search loop.
will add a map.


================
Comment at: lib/Target/AMDGPU/AMDGPULibFunc.h:240
+    // Library functions with unmangled name.
+    EI_READ_PIPE_2,
+    EI_READ_PIPE_4,
----------------
rampitec wrote:
> There is already EI_READ_PIPE above. Is it mangled? Then why these are unmangled?
clang does not emit mangled read_pipe functions. Will remove EI_READ_PIPE.


================
Comment at: lib/Target/AMDGPU/AMDGPULibFunc.h:366
 
-  void          reset();
+  static StringRef getUnmangledName(const StringRef &mangledName);
 
----------------
rampitec wrote:
> StringRef designed to be passed by value, here and below.
This comes from the original implementation. Will fix.


================
Comment at: test/CodeGen/AMDGPU/simplify-libcalls.ll:693
+entry:
+  %0 = bitcast i32 addrspace(1)* %ptr to i8 addrspace(1)*
+  %1 = addrspacecast i8 addrspace(1)* %0 to i8 addrspace(4)*
----------------
rampitec wrote:
> Run tests through opt -instnamer.
will fix.


https://reviews.llvm.org/D36831