[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-185
 // 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())
+static void locateCStrings(SparseBitVector<8> &BV, Value *Fmt, StringRef &FmtStr) {
+  if (!getConstantStringInfo(Fmt, FmtStr) || FmtStr.empty())
     return;
----------------
Seems like the only purpose of this change is to make the new StringRef available outside. It will be less confusing to just  move the call to getConstantStringInfo() out of this function, and pass the StringRef as an input parameter instead of output. Also, try not to change existing variable names unless it is really relevant. That reduces the number of lines affected by mostly non-functional changes.


================
Comment at: llvm/lib/Transforms/Utils/AMDGPUEmitPrintf.cpp:223
+
+static size_t alignUp(size_t Value, uint alignment) {
+  return (Value + alignment - 1) & ~(alignment - 1);
----------------
Needs consistent capitalization in the argument names. Since the rest of the code seems to prefer TitleCase, maybe alignment should start with "A"?


================
Comment at: llvm/lib/Transforms/Utils/AMDGPUEmitPrintf.cpp:369
+        // to alignment padding is not populated with any specific value
+        // here, I feel this would be safe as long as runtime is sync with
+        // the offsets.
----------------
Avoid first person comments. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150427