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

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: clang/docs/ReleaseNotes.rst:589
+  ``hostcall`` - printing happens during kernel execution via series of hostcalls,
+  The scheme requires the system to support pcie atomics.(default)
+  ``buffered`` - Scheme uses a debug buffer to populate printf varargs, does not
----------------
I thought the default was buffered, such that it would always work. Defaults that always work are better

Also, amdgpu-arch should probably learn to report if pcie atomics work to improve the autodetect


================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:4705-4707
+        // Force compiler error on invalid conversion specifiers
+        CmdArgs.push_back(
+            Args.MakeArgString("-Werror=format-invalid-specifier"));
----------------
I don't see why we would special case this 


================
Comment at: llvm/lib/Transforms/Utils/AMDGPUEmitPrintf.cpp:361
+    default:
+      llvm_unreachable("unexpected Integer Type");
+    }
----------------
It costs nothing to handle all types < 64. For larger just return the original value 


================
Comment at: llvm/lib/Transforms/Utils/AMDGPUEmitPrintf.cpp:368
+
+  if (Ty->isHalfTy())
+    return Builder.CreateFPExt(Arg, Builder.getDoubleTy());
----------------
Should avoid special casing this too, there's also bfloat to consider at a minimum 


================
Comment at: llvm/lib/Transforms/Utils/AMDGPUEmitPrintf.cpp:371
+
+  llvm_unreachable("unexpected type");
+}
----------------
Safest to just return the original value


================
Comment at: llvm/lib/Transforms/Utils/AMDGPUEmitPrintf.cpp:413
+
+    for (unsigned I = 0, E = WhatToStore.size(); I != E; ++I) {
+      Value *toStore = WhatToStore[I];
----------------
Range loop, index is unused 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150427