Merge main into fix-556: resolve CMakeLists.txt conflict

Author: pvelesko

CMakeLists.txt

@@ -710,12 +710,12 @@ message(STATUS "Generated HIP_OFFLOAD_LINK_OPTIONS: ${HIP_OFFLOAD_LINK_OPTIONS_I
 # TODO: Is there a better way to do this?
 add_library(deviceInternal INTERFACE)
 target_compile_options(deviceInternal INTERFACE
-  -x hip ${HIP_OFFLOAD_COMPILE_OPTIONS_BUILD_})
+  $<$<COMPILE_LANGUAGE:CXX>:-xhip> ${HIP_OFFLOAD_COMPILE_OPTIONS_BUILD_})
 target_link_libraries(deviceInternal INTERFACE CHIP)
 
 add_library(device INTERFACE)
 target_compile_options(device INTERFACE
-  -x hip ${HIP_OFFLOAD_COMPILE_OPTIONS_INSTALL_})
+  $<$<COMPILE_LANGUAGE:CXX>:-xhip> ${HIP_OFFLOAD_COMPILE_OPTIONS_INSTALL_})
 target_link_libraries(device INTERFACE CHIP)
 
 # same as device on chipStar but provides compatibility with AMD

HIPCC

@@ -20,7 +20,7 @@
  * DEALINGS IN THE SOFTWARE.
  */
 
-static void __attribute__((used)) _cl_print_str(__generic const char *S) {
+void _cl_print_str(__generic const char *S) {
   if (S == 0) {
     return;  /* Match AMD/nvidia: null %s prints nothing, format provides newline */
   }

bitcode/_cl_print_str.cl

@@ -43,47 +43,47 @@ uint4 _chip_tex2du(hipTextureObject_t TextureObject, float2 Pos);
 
 // ^^^ DECLARATIONS INTENTIONALLY WITHOUT DEFINITION ^^^
 
-static int4 __attribute__((used))
+int4
 _chip_tex1dfetchi_impl(image1d_t I, sampler_t S, int Pos) {
   return read_imagei(I, S, Pos);
 }
 
-static uint4 __attribute__((used))
+uint4
 _chip_tex1dfetchu_impl(image1d_t I, sampler_t S, int Pos) {
   return read_imageui(I, S, Pos);
 }
 
-static float4 __attribute__((used))
+float4
 _chip_tex1dfetchf_impl(image1d_t I, sampler_t S, int Pos) {
   return read_imagef(I, S, Pos);
 }
 
-static int4 __attribute__((used))
+int4
 _chip_tex1di_impl(image1d_t I, sampler_t S, float Pos) {
   return read_imagei(I, S, Pos);
 }
 
-static uint4 __attribute__((used))
+uint4
 _chip_tex1du_impl(image1d_t I, sampler_t S, float Pos) {
   return read_imageui(I, S, Pos);
 }
 
-static float4 __attribute__((used))
+float4
 _chip_tex1df_impl(image1d_t I, sampler_t S, float Pos) {
   return read_imagef(I, S, Pos);
 }
 
-static float4 __attribute__((used))
+float4
 _chip_tex2df_impl(image2d_t I, sampler_t S, float2 Pos) {
   return read_imagef(I, S, Pos);
 }
 
-static int4 __attribute__((used))
+int4
 _chip_tex2di_impl(image2d_t I, sampler_t S, float2 Pos) {
   return read_imagei(I, S, Pos);
 }
 
-static uint4 __attribute__((used))
+uint4
 _chip_tex2du_impl(image2d_t I, sampler_t S, float2 Pos) {
   return read_imageui(I, S, Pos);
 }

bitcode/texture.cl

@@ -314,21 +314,81 @@ HipPrintfToOpenCLPrintfPass::getOrCreateStrLiteralArg(const std::string &Str,
 Function *HipPrintfToOpenCLPrintfPass::getOrCreatePrintStringF() {
 
   if (GlobalValue *OldPrintStrF =
-          M_->getNamedValue(ORIG_PRINT_STRING_FUNC_NAME))
-    return cast<Function>(OldPrintStrF);
+          M_->getNamedValue(ORIG_PRINT_STRING_FUNC_NAME)) {
+    auto *Existing = cast<Function>(OldPrintStrF);
+    if (!Existing->isDeclaration())
+      return Existing;
+    // Declaration exists (e.g. inserted on a prior call before we got
+    // to define a body); fall through and define the body now.
+  }
 
-  auto *Int8Ty = IntegerType::get(M_->getContext(), 8);
+  auto &Ctx = M_->getContext();
+  auto *Int8Ty = IntegerType::get(Ctx, 8);
+  auto *Int32Ty = IntegerType::get(Ctx, 32);
+  auto *VoidTy = Type::getVoidTy(Ctx);
   PointerType *GenericCStrArgT =
       PointerType::get(Int8Ty, SPIRV_OPENCL_GENERIC_AS);
+  PointerType *ConstStrPtrT =
+      PointerType::get(Int8Ty, SPIRV_OPENCL_CONSTANT_AS);
 
-  FunctionType *PrintStrFTy = FunctionType::get(
-      Type::getVoidTy(M_->getContext()), {GenericCStrArgT}, false);
-
-  FunctionCallee PrintStrF =
-      M_->getOrInsertFunction(ORIG_PRINT_STRING_FUNC_NAME, PrintStrFTy);
-  cast<Function>(PrintStrF.getCallee())
-      ->setCallingConv(llvm::CallingConv::SPIR_FUNC);
-  return cast<Function>(PrintStrF.getCallee());
+  FunctionType *PrintStrFTy =
+      FunctionType::get(VoidTy, {GenericCStrArgT}, false);
+  Function *F = cast<Function>(
+      M_->getOrInsertFunction(ORIG_PRINT_STRING_FUNC_NAME, PrintStrFTy)
+          .getCallee());
+  F->setCallingConv(llvm::CallingConv::SPIR_FUNC);
+  F->setLinkage(llvm::GlobalValue::InternalLinkage);
+
+  // Define the body inline so the kernel module is self-contained for %s
+  // printf support. The historical implementation in bitcode/_cl_print_str.cl
+  // required `static __attribute__((used))`, which forced an `@llvm.used`
+  // entry into hipspv.bc. That collided with `@llvm.used` from HIP TUs
+  // (different element address space) at `-mlink-builtin-bitcode` time and
+  // broke any HIP code that also uses `__attribute__((used))` (rocThrust).
+  //
+  // Equivalent C:
+  //   void _cl_print_str(__generic const char *S) {
+  //     if (S == 0) return;
+  //     unsigned Pos = 0;
+  //     char C;
+  //     while ((C = S[Pos]) != 0) { printf("%c", C); ++Pos; }
+  //   }
+  BasicBlock *Entry = BasicBlock::Create(Ctx, "entry", F);
+  BasicBlock *Loop = BasicBlock::Create(Ctx, "loop", F);
+  BasicBlock *Body = BasicBlock::Create(Ctx, "body", F);
+  BasicBlock *Exit = BasicBlock::Create(Ctx, "exit", F);
+
+  Argument *S = F->getArg(0);
+
+  FunctionType *PrintfTy =
+      FunctionType::get(Int32Ty, {ConstStrPtrT}, /*isVarArg=*/true);
+  Function *Printf = cast<Function>(
+      M_->getOrInsertFunction("printf", PrintfTy).getCallee());
+
+  IRBuilder<> B(Entry);
+  Value *IsNull = B.CreateICmpEQ(S, ConstantPointerNull::get(GenericCStrArgT));
+  B.CreateCondBr(IsNull, Exit, Loop);
+
+  B.SetInsertPoint(Loop);
+  PHINode *Pos = B.CreatePHI(Int32Ty, 2);
+  Pos->addIncoming(ConstantInt::get(Int32Ty, 0), Entry);
+  Value *CharPtr = B.CreateGEP(Int8Ty, S, Pos);
+  Value *C = B.CreateLoad(Int8Ty, CharPtr);
+  Value *IsZero = B.CreateICmpEQ(C, ConstantInt::get(Int8Ty, 0));
+  B.CreateCondBr(IsZero, Exit, Body);
+
+  B.SetInsertPoint(Body);
+  Constant *PercentC = getOrCreateStrLiteralArg("%c", B);
+  CallInst *PrintfCall = B.CreateCall(Printf, {PercentC, C});
+  PrintfCall->setCallingConv(llvm::CallingConv::SPIR_FUNC);
+  Value *PosNext = B.CreateAdd(Pos, ConstantInt::get(Int32Ty, 1));
+  Pos->addIncoming(PosNext, Body);
+  B.CreateBr(Loop);
+
+  B.SetInsertPoint(Exit);
+  B.CreateRetVoid();
+
+  return F;
 }
 
 // Get called function from 'CI' call or return nullptr the call is indirect.

llvm_passes/HipPrintf.cpp

@@ -1419,14 +1419,28 @@ hipError_t chipstar::Context::free(void *Ptr) {
     // test suite excepts hipErrorInvalidValue. Go with the latter.
     return hipErrorInvalidValue;
 
-  if (AllocInfo->DevPtr)
-    ChipDev->AllocTracker->releaseMemReservation(AllocInfo->Size);
-  if (AllocInfo->MemoryType == hipMemoryTypeHost && AllocInfo->HostPtr &&
-      AllocInfo->HostPtr != AllocInfo->DevPtr)
-    std::free(AllocInfo->HostPtr);
+  // Save the fields we need before eraseRecord() deletes AllocInfo.
+  bool HasDevPtr = AllocInfo->DevPtr != nullptr;
+  size_t AllocSize = AllocInfo->Size;
+  bool IsHostNonMapped = (AllocInfo->MemoryType == hipMemoryTypeHost &&
+                          AllocInfo->HostPtr &&
+                          AllocInfo->HostPtr != AllocInfo->DevPtr);
+  void *HostPtrToFree = AllocInfo->HostPtr;
+
+  if (HasDevPtr)
+    ChipDev->AllocTracker->releaseMemReservation(AllocSize);
+
+  // Erase the record BEFORE freeing the underlying memory.  Freeing first
+  // returns the address to the allocator pool, so a concurrent allocation
+  // could obtain the same address and call recordAllocation() while the old
+  // entry is still in the tracker, causing a "Device pointer already
+  // recorded" abort (issue #1133).
+  ChipDev->AllocTracker->eraseRecord(AllocInfo); // AllocInfo deleted here.
+
+  if (IsHostNonMapped)
+    std::free(HostPtrToFree);
   else
     freeImpl(Ptr);
-  ChipDev->AllocTracker->eraseRecord(AllocInfo);
 
   return hipSuccess;
 }
@@ -1778,13 +1792,36 @@ static void mapHostAlloc(const void *Ptr,
 }
 
 ///////// Enqueue Operations //////////
+
+// Validate that a pointer expected to be a device allocation is actually
+// a known allocation in the tracker.  Throws hipErrorInvalidValue if not.
+static void validateDevicePtr(chipstar::AllocationTracker *AT, void *Ptr,
+                               bool IsDevice, const char *Role) {
+  if (!IsDevice)
+    return;
+  if (!AT->getAllocInfoCheckPtrRanges(Ptr))
+    CHIPERR_LOG_AND_THROW(std::string("hipMemcpy: invalid ") + Role +
+                              " device pointer.",
+                          hipErrorInvalidValue);
+}
+
 hipError_t chipstar::Queue::memCopy(void *Dst, const void *Src, size_t Size,
                                     hipMemcpyKind Kind) {
 
   std::shared_ptr<chipstar::Event> ChipEvent;
   unmapHostAlloc(Src);
   unmapHostAlloc(Dst);
 
+  auto *AT = getDevice()->AllocTracker;
+  bool DstDev = (Kind == hipMemcpyHostToDevice || Kind == hipMemcpyDeviceToDevice);
+  bool SrcDev = (Kind == hipMemcpyDeviceToHost || Kind == hipMemcpyDeviceToDevice);
+  if (Kind == hipMemcpyDefault) {
+    DstDev = AT->getAllocInfo(Dst) != nullptr;
+    SrcDev = AT->getAllocInfo(Src) != nullptr;
+  }
+  validateDevicePtr(AT, Dst, DstDev, "destination");
+  validateDevicePtr(AT, const_cast<void *>(Src), SrcDev, "source");
+
   ChipEvent = memCopyAsyncImpl(Dst, Src, Size, Kind);
 
   mapHostAlloc(Src);
@@ -1801,6 +1838,16 @@ void chipstar::Queue::memCopyAsync(void *Dst, const void *Src, size_t Size,
   unmapHostAlloc(Src);
   unmapHostAlloc(Dst);
 
+  auto *AT = getDevice()->AllocTracker;
+  bool DstDev = (Kind == hipMemcpyHostToDevice || Kind == hipMemcpyDeviceToDevice);
+  bool SrcDev = (Kind == hipMemcpyDeviceToHost || Kind == hipMemcpyDeviceToDevice);
+  if (Kind == hipMemcpyDefault) {
+    DstDev = AT->getAllocInfo(Dst) != nullptr;
+    SrcDev = AT->getAllocInfo(Src) != nullptr;
+  }
+  validateDevicePtr(AT, Dst, DstDev, "destination");
+  validateDevicePtr(AT, const_cast<void *>(Src), SrcDev, "source");
+
   ChipEvent = memCopyAsyncImpl(Dst, Src, Size, Kind);
 
   mapHostAlloc(Src);

src/CHIPBackend.cc

@@ -0,0 +1,13 @@
+# Regression test for #1001: C source files in a CMake target that links
+# against hip::device must not receive -x hip / -xhip compilation flags.
+# With the bug, helper.c (which uses 'class' as a C identifier) fails to
+# compile because it is treated as C++ / HIP source.
+cmake_minimum_required(VERSION 3.20 FATAL_ERROR)
+project(HipDeviceCSourceTrial
+  DESCRIPTION "Checking C sources are not compiled as HIP when using hip::device."
+  LANGUAGES C CXX)
+find_package(HIP CONFIG REQUIRED)
+
+# A mixed C+HIP executable that links against hip::device.
+add_executable(hip_c_source main.cpp helper.c)
+target_link_libraries(hip_c_source PRIVATE hip::device)

tests/post-install/HipDeviceCSource/CMakeLists.txt

@@ -0,0 +1,7 @@
+/* Pure C helper used by the regression test for issue #1001.
+   'class' is a reserved keyword in C++ / HIP but a valid identifier in C.
+   If this file is compiled with -x hip the compiler will reject it. */
+int get_value(void) {
+  int class = 42;
+  return class;
+}

tests/post-install/HipDeviceCSource/helper.c

@@ -0,0 +1,18 @@
+#include <hip/hip_runtime.h>
+#include <iostream>
+
+// Declared in helper.c (pure C, uses 'class' as a C identifier).
+// If -xhip/-x hip is applied to helper.c the compiler rejects it.
+extern "C" int get_value(void);
+
+int main() {
+  // Verify chipStar runtime is reachable.
+  int deviceCount = 0;
+  if (hipGetDeviceCount(&deviceCount) != hipSuccess)
+    return 1;
+
+  // Call the pure-C helper to confirm it compiled correctly.
+  int v = get_value();
+  std::cout << "get_value() = " << v << "\n";
+  return v != 42;
+}

tests/post-install/HipDeviceCSource/main.cpp

@@ -32,6 +32,15 @@ else
     echo "Skip: clang++ or g++ is required"
 fi
 echo
+echo "### Check hip::device with mixed C/C++ sources (#1001)"
+setup-test-dir
+cmake @CMAKE_CURRENT_SOURCE_DIR@/HipDeviceCSource \
+      -DCMAKE_PREFIX_PATH=@CMAKE_INSTALL_PREFIX@ \
+      -DCMAKE_CXX_COMPILER=${CXX}
+make VERBOSE=1
+echo "Success"
+echo
+
 echo "### check CMake-CUDA"
 setup-test-dir