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

[Sameer Sahasrabuddhe via Phabricator via llvm-commits]

sameerds added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/AMDGPUEmitPrintf.cpp:212
+// helper struct to package the string related data
+typedef struct S {
+  std::string Str;
----------------
you can just say "struct StringData"


================
Comment at: llvm/lib/Transforms/Utils/AMDGPUEmitPrintf.cpp:218-219
+
+  S(std::string str = "", bool IC = true, Value *RS = nullptr,
+    Value *AS = nullptr)
+      : Str(str), isConst(IC), RealSize(RS), AlignedSize(AS) {}
----------------
Every argument has a default value. Why not simply assign them as member initializers?


================
Comment at: llvm/lib/Transforms/Utils/AMDGPUEmitPrintf.cpp:238
+
+  // First 8 bytes to be reserved for control dword
+  size_t BufSize = 4;
----------------
Should this say 4?


================
Comment at: llvm/lib/Transforms/Utils/AMDGPUEmitPrintf.cpp:256
+
+  StringRef ArgStr;
+  for (size_t i = 1; i < Args.size(); i++) {
----------------
move this closer to first use


================
Comment at: llvm/lib/Transforms/Utils/AMDGPUEmitPrintf.cpp:313
+                                  SparseBitVector<8> &SpecIsCString,
+                                  SmallVector<StringData, 8> &StringContents,
+                                  bool isConstFmtStr) {
----------------
function argument should use SmallVectorImpl, and not SmallVector


================
Comment at: llvm/lib/Transforms/Utils/AMDGPUEmitPrintf.cpp:321
+    SmallVector<Value *, 32> WhatToStore;
+    if ((i == 0) || SpecIsCString.test(i)) {
+      if ((*StrIt).isConst) {
----------------
This "if (...) { ... }" has a really long body. Can it be moved into a separate function for readability?


================
Comment at: llvm/lib/Transforms/Utils/AMDGPUEmitPrintf.cpp:324
+        Str = (*StrIt).Str;
+        const uint64_t ReadSize = 4;
+
----------------
Try to move declarations to the smallest scope that they are used in. For ReadSize, this seems to be the while loop later.


================
Comment at: llvm/lib/Transforms/Utils/AMDGPUEmitPrintf.cpp:352
+
+          // TODO: Should not bothering aligning up.
+          if (ReadNow < ReadSize)
----------------
typo: bother


================
Comment at: llvm/lib/Transforms/Utils/AMDGPUEmitPrintf.cpp:426-427
+
+    // The buffered version still follows OpenCL printf standards for
+    // printf return value, i.e 0 on success, 1 on failure.
+    ConstantPointerNull *zeroIntPtr =
----------------
So we cannot use a buffered printf in HIP?


================
Comment at: llvm/lib/Transforms/Utils/AMDGPUEmitPrintf.cpp:451
+        {ArgSize,
+         ConstantInt::get(Int1Ty, !FmtStr.empty() ? 1 : 0, false),
+         ConstantInt::get(Int1Ty, 0, false)});
----------------
FmtStr.empty() gets checked in multiple places. Can this be captured as one or more boolean variables with meaningful names?


================
Comment at: llvm/lib/Transforms/Utils/AMDGPUEmitPrintf.cpp:487
+      if(metaD->getNumOperands() == 0) {
+        MDString *fmtStrArray = MDString::get(Ctx, "0:0:deadbeef,\"\"");
+        MDNode *myMD = MDNode::get(Ctx, fmtStrArray);
----------------
Probably not a good choice if it shows up where the user can see it. It's also not very informative. A more descriptive string will be better.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150427