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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
