Skip to content

Conversation

@sarnex
Copy link
Member

@sarnex sarnex commented Nov 10, 2025

We see some libomptarget test failures if we use the default global AS.

See #166459 for more info.

Signed-off-by: Nick Sarnie <nick.sarnie@intel.com>
@sarnex sarnex marked this pull request as ready for review November 10, 2025 20:01
@llvmbot llvmbot added clang Clang issues not falling into any other category flang:openmp clang:openmp OpenMP related changes to Clang labels Nov 10, 2025
@llvmbot
Copy link
Member

llvmbot commented Nov 10, 2025

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-flang-openmp

Author: Nick Sarnie (sarnex)

Changes

We see some libomptarget test failures if we use the default global as.

See #166459 for more info.


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

2 Files Affected:

  • (modified) clang/test/OpenMP/force-usm.c (+1-1)
  • (modified) llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp (+9-2)
diff --git a/clang/test/OpenMP/force-usm.c b/clang/test/OpenMP/force-usm.c
index 45c0e28b525da..5c63a9a5e7004 100644
--- a/clang/test/OpenMP/force-usm.c
+++ b/clang/test/OpenMP/force-usm.c
@@ -46,7 +46,7 @@ int main(void) {
 // CHECK-USM-NEXT:    br i1 [[EXEC_USER_CODE]], label [[USER_CODE_ENTRY:%.*]], label [[WORKER_EXIT:%.*]]
 // CHECK-USM:       user_code.entry:
 // CHECK-USM-NEXT:    store i32 1, ptr [[TMP0]], align 4
-// CHECK-USM-NEXT:    [[TMP2:%.*]] = load ptr, ptr addrspace(1) @pGI_decl_tgt_ref_ptr, align 8
+// CHECK-USM-NEXT:    [[TMP2:%.*]] = load ptr, ptr @pGI_decl_tgt_ref_ptr, align 8
 // CHECK-USM-NEXT:    [[TMP3:%.*]] = load ptr, ptr [[TMP2]], align 8
 // CHECK-USM-NEXT:    store i32 2, ptr [[TMP3]], align 4
 // CHECK-USM-NEXT:    call void @__kmpc_target_deinit()
diff --git a/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp b/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
index 7dc32fda0eed6..44b4195982c49 100644
--- a/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
+++ b/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
@@ -8472,8 +8472,15 @@ GlobalVariable *OpenMPIRBuilder::getOrCreateInternalVariable(
     // create different versions of the function for different OMP internal
     // variables.
     const DataLayout &DL = M.getDataLayout();
-    unsigned AddressSpaceVal =
-        AddressSpace ? *AddressSpace : DL.getDefaultGlobalsAddressSpace();
+    // TODO: Investigate why AMDGPU expects AS 0 for globals even though the
+    // default global AS is 1.
+    // See double-target-call-with-declare-target.f90 and
+    // declare-target-vars-in-target-region.f90 libomptarget
+    // tests.
+    unsigned AddressSpaceVal = AddressSpace ? *AddressSpace
+                               : M.getTargetTriple().isAMDGPU()
+                                   ? 0
+                                   : DL.getDefaultGlobalsAddressSpace();
     auto Linkage = this->M.getTargetTriple().getArch() == Triple::wasm32
                        ? GlobalValue::InternalLinkage
                        : GlobalValue::CommonLinkage;

@sarnex sarnex requested review from jplehr and tblah November 10, 2025 20:02
@Kewen12
Copy link
Contributor

Kewen12 commented Nov 11, 2025

Hi! I tested this PR and the following fortran tests are no longer failed:

libomptarget :: amdgcn-amd-amdhsa :: offloading/fortran/declare-target-vars-in-target-region.f90
libomptarget :: amdgcn-amd-amdhsa :: offloading/fortran/double-target-call-with-declare-target.f90
libomptarget :: amdgcn-amd-amdhsa :: offloading/fortran/target-map-declare-target-link-common-block.f90

I am going to land this PR to unblock our bot. https://lab.llvm.org/staging/#/builders/105

@Kewen12 Kewen12 self-requested a review November 11, 2025 03:32
@Kewen12 Kewen12 merged commit ae50366 into llvm:main Nov 11, 2025
16 checks passed
@sarnex
Copy link
Member Author

sarnex commented Nov 11, 2025

Thanks @Kewen12!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

clang:openmp OpenMP related changes to Clang clang Clang issues not falling into any other category flang:openmp

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants