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

[Sameer Sahasrabuddhe via Phabricator via cfe-commits]

sameerds added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/AMDGPUEmitPrintf.cpp:184
 // specify a string, i.e, the "%s" specifier with optional '*' characters.
-static void locateCStrings(SparseBitVector<8> &BV, Value *Fmt) {
-  StringRef Str;
-  if (!getConstantStringInfo(Fmt, Str) || Str.empty())
-    return;
+static void locateCStrings(SparseBitVector<8> &BV, Value *Fmt, StringRef &Str) {
 
----------------
Where is Fmt used in this function? Also, StringRef is passed by value. It's already a lightweight reference, as demonstrated by its name.


================
Comment at: llvm/lib/Transforms/Utils/AMDGPUEmitPrintf.cpp:210
+// helper struct to package the string related data
+typedef struct StringData {
+  std::string Str = "";
----------------
typedef is not required for this in C++


================
Comment at: llvm/lib/Transforms/Utils/AMDGPUEmitPrintf.cpp:486-487
+                              {ConstantInt::get(Int32Ty, 8)});
+    }
+    else {
+      // Include a dummy metadata instance in case of only non constant
----------------
else should be on the same line as the closing brace. Please do run clang-format on the entire change.


================
Comment at: llvm/lib/Transforms/Utils/AMDGPUEmitPrintf.cpp:458
+    auto CreateControlDWord = M->getOrInsertFunction(
+        StringRef("__ockl_create_control_dword"), Builder.getInt32Ty(),
+        Builder.getInt32Ty(), Int1Ty, Int1Ty);
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150427