[clang] Recover necessary AddrSpaceCast #119246

JOE1994 · 2024-12-09T18:48:35Z

A necessary AddrSpaceCast was wrongfully deleted in 5c91b28 . Recover the AddrSpaceCast.

This fixes #86791 .

A necessary AddrSpaceCast was wrongfully deleted in 5c91b28 . Recover the AddrSpaceCast. This fixes llvm#86791 .

llvmbot · 2024-12-09T18:49:09Z

@llvm/pr-subscribers-clang-codegen

Author: Youngsuk Kim (JOE1994)

Changes

A necessary AddrSpaceCast was wrongfully deleted in 5c91b28 . Recover the AddrSpaceCast.

This fixes #86791 .

Full diff: https://github.com/llvm/llvm-project/pull/119246.diff

2 Files Affected:

(modified) clang/lib/CodeGen/ItaniumCXXABI.cpp (+3)
(added) clang/test/OpenMP/amdgpu_threadprivate.cpp (+11)

diff --git a/clang/lib/CodeGen/ItaniumCXXABI.cpp b/clang/lib/CodeGen/ItaniumCXXABI.cpp
index 8cbd09d02c7556..0abea335ad69e4 100644
--- a/clang/lib/CodeGen/ItaniumCXXABI.cpp
+++ b/clang/lib/CodeGen/ItaniumCXXABI.cpp
@@ -3302,6 +3302,9 @@ void ItaniumCXXABI::EmitThreadLocalInitFuncs(
       CharUnits Align = CGM.getContext().getDeclAlign(VD);
       Val = Builder.CreateAlignedLoad(Var->getValueType(), Val, Align);
     }
+    if (Val->getType() != Wrapper->getReturnType()) {
+      Val = Builder.CreateAddrSpaceCast(Val, Wrapper->getReturnType());
+    }
 
     Builder.CreateRet(Val);
   }
diff --git a/clang/test/OpenMP/amdgpu_threadprivate.cpp b/clang/test/OpenMP/amdgpu_threadprivate.cpp
new file mode 100644
index 00000000000000..5b15255ee62d4a
--- /dev/null
+++ b/clang/test/OpenMP/amdgpu_threadprivate.cpp
@@ -0,0 +1,11 @@
+// REQUIRES: asserts
+
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -target-cpu x86-64 -disable-llvm-passes -fopenmp-targets=amdgcn-amd-amdhsa -x c++ -emit-llvm-bc %s -o %t-x86-host.bc
+// RUN: %clang_cc1 -triple amdgcn-amd-amdhsa -aux-triple x86_64-unknown-linux-gnu -target-cpu gfx906 -fopenmp -nogpulib -fopenmp-is-target-device -fopenmp-host-ir-file-path %t-x86-host.bc -x c++ %s
+
+// Don't crash with assertions build.
+int MyGlobVar;
+#pragma omp threadprivate(MyGlobVar)
+int main() {
+  MyGlobVar = 1;
+}

llvmbot · 2024-12-09T18:49:10Z

@llvm/pr-subscribers-backend-amdgpu

Author: Youngsuk Kim (JOE1994)

Changes

A necessary AddrSpaceCast was wrongfully deleted in 5c91b28 . Recover the AddrSpaceCast.

This fixes #86791 .

Full diff: https://github.com/llvm/llvm-project/pull/119246.diff

2 Files Affected:

(modified) clang/lib/CodeGen/ItaniumCXXABI.cpp (+3)
(added) clang/test/OpenMP/amdgpu_threadprivate.cpp (+11)

diff --git a/clang/lib/CodeGen/ItaniumCXXABI.cpp b/clang/lib/CodeGen/ItaniumCXXABI.cpp
index 8cbd09d02c7556..0abea335ad69e4 100644
--- a/clang/lib/CodeGen/ItaniumCXXABI.cpp
+++ b/clang/lib/CodeGen/ItaniumCXXABI.cpp
@@ -3302,6 +3302,9 @@ void ItaniumCXXABI::EmitThreadLocalInitFuncs(
       CharUnits Align = CGM.getContext().getDeclAlign(VD);
       Val = Builder.CreateAlignedLoad(Var->getValueType(), Val, Align);
     }
+    if (Val->getType() != Wrapper->getReturnType()) {
+      Val = Builder.CreateAddrSpaceCast(Val, Wrapper->getReturnType());
+    }
 
     Builder.CreateRet(Val);
   }
diff --git a/clang/test/OpenMP/amdgpu_threadprivate.cpp b/clang/test/OpenMP/amdgpu_threadprivate.cpp
new file mode 100644
index 00000000000000..5b15255ee62d4a
--- /dev/null
+++ b/clang/test/OpenMP/amdgpu_threadprivate.cpp
@@ -0,0 +1,11 @@
+// REQUIRES: asserts
+
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -target-cpu x86-64 -disable-llvm-passes -fopenmp-targets=amdgcn-amd-amdhsa -x c++ -emit-llvm-bc %s -o %t-x86-host.bc
+// RUN: %clang_cc1 -triple amdgcn-amd-amdhsa -aux-triple x86_64-unknown-linux-gnu -target-cpu gfx906 -fopenmp -nogpulib -fopenmp-is-target-device -fopenmp-host-ir-file-path %t-x86-host.bc -x c++ %s
+
+// Don't crash with assertions build.
+int MyGlobVar;
+#pragma omp threadprivate(MyGlobVar)
+int main() {
+  MyGlobVar = 1;
+}

llvmbot · 2024-12-09T18:49:10Z

@llvm/pr-subscribers-clang

Author: Youngsuk Kim (JOE1994)

Changes

A necessary AddrSpaceCast was wrongfully deleted in 5c91b28 . Recover the AddrSpaceCast.

This fixes #86791 .

Full diff: https://github.com/llvm/llvm-project/pull/119246.diff

2 Files Affected:

(modified) clang/lib/CodeGen/ItaniumCXXABI.cpp (+3)
(added) clang/test/OpenMP/amdgpu_threadprivate.cpp (+11)

diff --git a/clang/lib/CodeGen/ItaniumCXXABI.cpp b/clang/lib/CodeGen/ItaniumCXXABI.cpp
index 8cbd09d02c7556..0abea335ad69e4 100644
--- a/clang/lib/CodeGen/ItaniumCXXABI.cpp
+++ b/clang/lib/CodeGen/ItaniumCXXABI.cpp
@@ -3302,6 +3302,9 @@ void ItaniumCXXABI::EmitThreadLocalInitFuncs(
       CharUnits Align = CGM.getContext().getDeclAlign(VD);
       Val = Builder.CreateAlignedLoad(Var->getValueType(), Val, Align);
     }
+    if (Val->getType() != Wrapper->getReturnType()) {
+      Val = Builder.CreateAddrSpaceCast(Val, Wrapper->getReturnType());
+    }
 
     Builder.CreateRet(Val);
   }
diff --git a/clang/test/OpenMP/amdgpu_threadprivate.cpp b/clang/test/OpenMP/amdgpu_threadprivate.cpp
new file mode 100644
index 00000000000000..5b15255ee62d4a
--- /dev/null
+++ b/clang/test/OpenMP/amdgpu_threadprivate.cpp
@@ -0,0 +1,11 @@
+// REQUIRES: asserts
+
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -target-cpu x86-64 -disable-llvm-passes -fopenmp-targets=amdgcn-amd-amdhsa -x c++ -emit-llvm-bc %s -o %t-x86-host.bc
+// RUN: %clang_cc1 -triple amdgcn-amd-amdhsa -aux-triple x86_64-unknown-linux-gnu -target-cpu gfx906 -fopenmp -nogpulib -fopenmp-is-target-device -fopenmp-host-ir-file-path %t-x86-host.bc -x c++ %s
+
+// Don't crash with assertions build.
+int MyGlobVar;
+#pragma omp threadprivate(MyGlobVar)
+int main() {
+  MyGlobVar = 1;
+}

s-barannikov

May it be possible that the wrapper function's return type is invalid and the cast isn't needed?

clang/lib/CodeGen/ItaniumCXXABI.cpp

clang/test/OpenMP/amdgpu_threadprivate.cpp

clang/lib/CodeGen/ItaniumCXXABI.cpp

shiltian · 2024-12-12T15:07:04Z

clang/test/OpenMP/amdgpu_threadprivate.cpp

+
+// Don't crash with assertions build.
+int MyGlobVar;
+#pragma omp threadprivate(MyGlobVar)


This should not crash for sure, but I wonder is this a valid use of OpenMP? I don't know what I should expect from this semantics.

JOE1994 · 2024-12-12T15:57:42Z

May it be possible that the wrapper function's return type is invalid and the cast isn't needed?

LLVM-IR (from llvm without assertions, before this revision)
shows that the wrapper function returns ptr while MyGlobVar has addrspace(1).

$thread-local wrapper routine for MyGlobVar = comdat any

@MyGlobVar = external thread_local addrspace(1) global i32, align 4
@__oclc_ABI_version = weak_odr hidden local_unnamed_addr addrspace(4) constant i32 500

define weak_odr hidden noundef ptr @thread-local wrapper routine for MyGlobVar() local_unnamed_addr #0 comdat {
  %1 = tail call align 4 ptr addrspace(1) @llvm.threadlocal.address.p1(ptr addrspace(1) align 4 @MyGlobVar)
  ret ptr addrspace(1) %1
}

declare nonnull ptr addrspace(1) @llvm.threadlocal.address.p1(ptr addrspace(1) nonnull) #1

attributes #0 = { nounwind "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="gfx906" "target-features"="+16-bit-insts,+ci-insts,+dl-insts,+dot1-insts,+dot10-insts,+dot2-insts,+dot7-insts,+dpp,+gfx8-insts,+gfx9-insts,+s-memrealtime,+s-memtime-inst,+wavefrontsize64" "uniform-work-group-size"="false" }
attributes #1 = { mustprogress nocallback nofree nosync nounwind speculatable willreturn memory(none) }

Updating the wrapper function's return type as below also fixes the error:

--- a/clang/lib/CodeGen/ItaniumCXXABI.cpp
+++ b/clang/lib/CodeGen/ItaniumCXXABI.cpp
@@ -3078,6 +3078,10 @@ ItaniumCXXABI::getOrCreateThreadLocalWrapper(const VarDecl *VD,
       getContext().getPointerType(RetQT), FunctionArgList());

   llvm::FunctionType *FnTy = CGM.getTypes().GetFunctionType(FI);
+  // Adjust wrapper function's ret type to have matching llvm addrspace with Val.
+  if (llvm::Type *WrapperRetTy = FnTy->getReturnType();
+      WrapperRetTy->isPointerTy() && Val->getType()->isPointerTy() && WrapperRetTy != Val->getType())
+    FnTy = llvm::FunctionType::get(Val->getType(), FnTy->params(), FnTy->isVarArg());
   llvm::Function *Wrapper =
       llvm::Function::Create(FnTy, getThreadLocalWrapperLinkage(VD, CGM),
                              WrapperName.str(), &CGM.getModule());

As per feedback from reviewers.

s-barannikov · 2024-12-12T16:13:33Z

Updating the wrapper function's return type as below also fixes the error:

I mean, why do we have to watch for type mismatches and fix them instead of creating the correct type in the first place?
There is getPointerType(RetQT) just above, shouldn't it be something like getPointerType(RetQT, <correct address space>)?
Or RetQT should have had the correct address space, but for some reason it was lost?

JOE1994 · 2024-12-12T16:22:50Z

I mean, why do we have to watch for type mismatches and fix them instead of creating the correct type in the first place?
There is getPointerType(RetQT) just above, shouldn't it be something like getPointerType(RetQT, )?
Or RetQT should have had the correct address space, but for some reason it was lost?

I totally agree with you.
I'm unfamiliar with Clang Qualtypes
and how clang QualTypes' addrspace info
match to llvm types' addrspace info,
so I shared a quick dirty fix to at least share a direction.

s-barannikov · 2024-12-12T16:30:12Z

I'm unfamiliar with Clang Qualtypes

I see. Well, neither am I :) I brought this up in case someone else could suggest the preferred way of fixing the issue.
I don't have objections against the current approach, but I'm not qualified to review it either.

arsenm · 2024-12-13T01:27:24Z

I mean, why do we have to watch for type mismatches and fix them instead of creating the correct type in the first place? There is getPointerType(RetQT) just above, shouldn't it be something like getPointerType(RetQT, <correct address space>)? Or RetQT should have had the correct address space, but for some reason it was lost?

Correct, this is generally how things should work. There are select contexts where a cast is necessary.

I'm unfamiliar with Clang Qualtypes and how clang QualTypes' addrspace info match to llvm types' addrspace info, so I shared a quick dirty fix to at least share a direction.

This depends where the value was created from. Generally you just query getTargetAddressSpace

efriedma-quic · 2024-12-13T19:39:40Z

The function in question is part of the Itanium C++ ABI; the ABI itself doesn't say anything about address-spaces, so by default we assume everything related to the C++ ABI is in the flat address-space. If we start messing with that, we'll need to write a specification somewhere to document exactly which functions use which address-spaces.

So I think casting here is appropriate; we can revisit if someone ever wants to define a "GPU Itanium ABI" with different address-spaces.

The change to ItaniumCXXABI.cpp looks fine (please address the review comments on the regression test).

clang/test/OpenMP/amdgpu_threadprivate.cpp

arsenm · 2024-12-17T02:11:07Z

clang/test/OpenMP/amdgpu_threadprivate.cpp

+
+// CHECK:       @MyGlobVar = external thread_local addrspace(1) global i32, align 4
+// CHECK:       define weak_odr hidden noundef ptr @_ZTW9MyGlobVar() #0 comdat {
+// CHECK-NEXT:    %1 = call align 4 ptr addrspace(1) @llvm.threadlocal.address.p1(ptr addrspace(1) align 4 @MyGlobVar)


Do we support this intrinsic?

(For AMDGPU codegen) Doesn't seem so to me.

The intrinsic was added in 9701053

[clang] Recover necessary AddrSpaceCast

9dbcf65

A necessary AddrSpaceCast was wrongfully deleted in 5c91b28 . Recover the AddrSpaceCast. This fixes llvm#86791 .

JOE1994 requested review from shiltian, s-barannikov and fel-cab December 9, 2024 18:48

llvmbot added clang Clang issues not falling into any other category backend:AMDGPU clang:codegen IR generation bugs: mangling, exceptions, etc. clang:openmp OpenMP related changes to Clang labels Dec 9, 2024

s-barannikov reviewed Dec 9, 2024

View reviewed changes

clang/lib/CodeGen/ItaniumCXXABI.cpp Outdated Show resolved Hide resolved

arsenm reviewed Dec 12, 2024

View reviewed changes

clang/test/OpenMP/amdgpu_threadprivate.cpp Outdated Show resolved Hide resolved

clang/test/OpenMP/amdgpu_threadprivate.cpp Show resolved Hide resolved

clang/lib/CodeGen/ItaniumCXXABI.cpp Outdated Show resolved Hide resolved

shiltian reviewed Dec 12, 2024

View reviewed changes

Avoid redundant type check before AddrSpaceCast

4af1ba5

As per feedback from reviewers.

Add FileCheck checks

6466437

arsenm reviewed Dec 16, 2024

View reviewed changes

clang/test/OpenMP/amdgpu_threadprivate.cpp Outdated Show resolved Hide resolved

Remove host RUN line, strip '-x c++'

169f744

arsenm approved these changes Dec 17, 2024

View reviewed changes

JOE1994 merged commit b4c1ded into llvm:main Dec 17, 2024
8 checks passed

JOE1994 deleted the fix_for_86791 branch December 17, 2024 13:38

[clang] Recover necessary AddrSpaceCast #119246

[clang] Recover necessary AddrSpaceCast #119246

Uh oh!

Conversation

JOE1994 commented Dec 9, 2024

Uh oh!

llvmbot commented Dec 9, 2024

Uh oh!

llvmbot commented Dec 9, 2024

Uh oh!

llvmbot commented Dec 9, 2024

Uh oh!

s-barannikov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

shiltian Dec 12, 2024

Choose a reason for hiding this comment

Uh oh!

JOE1994 commented Dec 12, 2024

Uh oh!

s-barannikov commented Dec 12, 2024

Uh oh!

JOE1994 commented Dec 12, 2024

Uh oh!

s-barannikov commented Dec 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arsenm commented Dec 13, 2024

Uh oh!

efriedma-quic commented Dec 13, 2024

Uh oh!

Uh oh!

arsenm Dec 17, 2024

Choose a reason for hiding this comment

Uh oh!

JOE1994 Dec 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

s-barannikov commented Dec 12, 2024 •

edited

Loading

JOE1994 Dec 17, 2024 •

edited

Loading