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

Mon Jun 5 16:17:46 PDT 2023

arsenm added inline comments.

================
Comment at: llvm/lib/Transforms/Utils/AMDGPUEmitPrintf.cpp:371
+        // the offsets.
+        uint64_t DstAlign = (i == 0) ? 4 : 8;
+        Builder.CreateMemCpy(PtrToStore, /*DstAlign*/ Align(DstAlign), Args[i],
----------------
I don't follow this alignment logic, but I guess the optimizer will figure it out later anyway so I suppose the Align(1) was fine.

Also should use Align.

================
Comment at: llvm/lib/Transforms/Utils/AMDGPUEmitPrintf.cpp:373
+        Builder.CreateMemCpy(PtrToStore, /*DstAlign*/ Align(DstAlign), Args[i],
+                             /*SrcAlign*/ Align(1), StrIt->RealSize);
+
----------------
The source align is known

================
Comment at: llvm/lib/Transforms/Utils/AMDGPUEmitPrintf.cpp:386-387
+    } else {
+      auto IntTy = dyn_cast<IntegerType>(Args[i]->getType());
+      if (IntTy && IntTy->getBitWidth() == 32)
+        WhatToStore.push_back(
----------------
isIntegerTy(32).

I also do not understand why only 32-bit integers would be promoted to 64-bit (or why this would be zext). This doesn't match varargs ABI handling, where everything smaller would be promoted to i32.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150427