[PATCH] D150427: [AMDGPU] Non hostcall printf support for HIP

[Vikram Hegde via Phabricator via cfe-commits]

vikramRH marked 11 inline comments as done.
vikramRH added inline comments.


================
Comment at: clang/test/CodeGenHIP/printf_nonhostcall.cpp:64
+// CHECK-NEXT:    [[PRINTBUFFNEXTPTR4:%.*]] = getelementptr inbounds i8, ptr addrspace(1) [[PRINTBUFFNEXTPTR3]], i64 [[TMP13]]
+// CHECK-NEXT:    [[TMP21:%.*]] = ptrtoint ptr [[TMP1]] to i64
+// CHECK-NEXT:    store i64 [[TMP21]], ptr addrspace(1) [[PRINTBUFFNEXTPTR4]], align 8
----------------
arsenm wrote:
> Can directly store the pointer, don't ptrtoint? Should have some other pointer address spaces tested, the spec is quiet on how %p is supposed to work with different sized pointers 
Added few more address space cases in foo2(), do you think we need more ?


================
Comment at: clang/test/CodeGenHIP/printf_nonhostcall.cpp:228
+  return printf(s, 10);
+}
----------------
arsenm wrote:
> Need some vector tests, especially 3 x vectors 
vectors are not yet supported as in the hostcall case since the code currently runs for HIP only. I plan to port the OpenCL lowering also here along with hostcall support . we would need to extend hostcall and this implementation to support vectors as part of the porting activity.


================
Comment at: llvm/lib/Transforms/Utils/AMDGPUEmitPrintf.cpp:211
+struct StringData {
+  std::string Str = "";
+  bool isConst = true;
----------------
arsenm wrote:
> arsenm wrote:
> > Don't need = ""
> Can't you just use the raw StringRef out of getConstantStringInfo
The structure just caches the string calculated from getConstantStringInfo


================
Comment at: llvm/lib/Transforms/Utils/AMDGPUEmitPrintf.cpp:381
+    } else {
+      if (Args[i]->getType()->isDoubleTy())
+        WhatToStore.push_back(Args[i]);
----------------
arsenm wrote:
> Don't see why isDoubleTy would be special cased here and not part of fitArgInto64Bits
The redundant cast instructions and this case were due to function "fitArgInto64Bits", which I reused from hostcall Implementation and did not want to modify. This dependency has been removed now and I have inlined the code


================
Comment at: llvm/lib/Transforms/Utils/AMDGPUEmitPrintf.cpp:458
+    auto CreateControlDWord = M->getOrInsertFunction(
+        StringRef("__ockl_create_control_dword"), Builder.getInt32Ty(),
+        Builder.getInt32Ty(), Int1Ty, Int1Ty);
----------------
arsenm wrote:
> sameerds wrote:
> > vikramRH wrote:
> > > arsenm wrote:
> > > > vikramRH wrote:
> > > > > arsenm wrote:
> > > > > > Do we really need another ockl control variable for this? Why isn't it a parameter? printf=stdout always 
> > > > > There are certain HIP API's such as "hip_assert" that output to stderr. currently such API's are supported via hostcalls. Although this implementation does not currently support the API's ,its kept as an option. 
> > > > Right but the way to handle that would be a parameter for where to output, not an externally set global 
> > > I am not clear here, you expect additional inputs to device lib function ?
> > @arsenm, this "control word" written into the buffer. In that sense, it is indeed a parameter passed from device to host as part of the printf packet. It is not yet another global variable.
> I'm not following why this part requires introducing another call into device libs. It's expanding this not-really defined API. I'd expect this to be a trivial function we can expand inline here and write directly into the parameter buffer?
I started the implementation keeping in mind the device side asserts currenlty supported via hostcall. They use explicit calls to such device lib functions in their definitions. I could not support them now but these would be required if I ever decide to. also I never ended up inlining this call. do you think its really necessary ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150427