Skip to content

Conversation

anchuraj
Copy link
Contributor

@anchuraj anchuraj commented Aug 20, 2025

Fix enables atomic control options in non-fc1 mode. The changes, #151579, #150860 enabled it only in fc1 mode.

The options used are:
-f[no-]atomic-remote-memory, -f[no-]atomic-fine-grained-memory,
-f[no-]atomic-ignore-denormal-mode.
Legacy option -m[no-]unsafe-fp-atomics is aliased to
-f[no-]ignore-denormal-mode.
More details can be found in #102569.

@anchuraj anchuraj requested review from tarunprabhu and tblah August 20, 2025 22:15
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' flang:driver flang Flang issues not falling into any other category labels Aug 20, 2025
@anchuraj anchuraj requested a review from skatrak August 20, 2025 22:15
@llvmbot
Copy link
Member

llvmbot commented Aug 20, 2025

@llvm/pr-subscribers-flang-driver

@llvm/pr-subscribers-clang-driver

Author: Anchu Rajendran S (anchuraj)

Changes

Fix enables options in non-fc1 mode. The changes, #151579, #150860 enabled it only in fc1 mode.


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

2 Files Affected:

  • (modified) clang/lib/Driver/ToolChains/Flang.cpp (+8-1)
  • (modified) flang/test/Driver/atomic-control-options.f90 (+4)
diff --git a/clang/lib/Driver/ToolChains/Flang.cpp b/clang/lib/Driver/ToolChains/Flang.cpp
index 65391033c2b9f..1535f4cebf436 100644
--- a/clang/lib/Driver/ToolChains/Flang.cpp
+++ b/clang/lib/Driver/ToolChains/Flang.cpp
@@ -534,7 +534,14 @@ void Flang::addTargetOptions(const ArgList &Args,
   }
 
   Args.addAllArgs(CmdArgs,
-                  {options::OPT_fverbose_asm, options::OPT_fno_verbose_asm});
+                  {options::OPT_fverbose_asm, options::OPT_fno_verbose_asm,
+                   options::OPT_fatomic_ignore_denormal_mode,
+                   options::OPT_fno_atomic_ignore_denormal_mode,
+                   options::OPT_fatomic_fine_grained_memory,
+                   options::OPT_fno_atomic_fine_grained_memory,
+                   options::OPT_fatomic_remote_memory,
+                   options::OPT_fno_atomic_remote_memory,
+                   options::OPT_munsafe_fp_atomics});
 }
 
 void Flang::addOffloadOptions(Compilation &C, const InputInfoList &Inputs,
diff --git a/flang/test/Driver/atomic-control-options.f90 b/flang/test/Driver/atomic-control-options.f90
index cb382f96a9d5f..3251896033c25 100644
--- a/flang/test/Driver/atomic-control-options.f90
+++ b/flang/test/Driver/atomic-control-options.f90
@@ -1,8 +1,12 @@
 ! REQUIRES: amdgpu-registered-target
 ! RUN: %flang_fc1 -emit-llvm -triple amdgcn-amd-amdhsa -fopenmp -fopenmp-is-device -munsafe-fp-atomics %s -o -|FileCheck -check-prefix=UNSAFE-FP-ATOMICS %s
+! RUN: %flang --offload-arch=gfx90a --offload-device-only -fopenmp -emit-llvm -S %s -munsafe-fp-atomics -o -|FileCheck -check-prefix=UNSAFE-FP-ATOMICS %s
 ! RUN: %flang_fc1 -emit-llvm -triple amdgcn-amd-amdhsa -fopenmp -fopenmp-is-device -fatomic-ignore-denormal-mode %s -o -|FileCheck -check-prefix=IGNORE-DENORMAL-MODE %s
+! RUN: %flang --offload-arch=gfx90a --offload-device-only -fopenmp -emit-llvm -S %s -fatomic-ignore-denormal-mode -o -|FileCheck -check-prefix=IGNORE-DENORMAL-MODE %s
 ! RUN: %flang_fc1 -emit-llvm -triple amdgcn-amd-amdhsa -fopenmp -fopenmp-is-device -fatomic-fine-grained-memory %s -o -|FileCheck -check-prefix=FINE-GRAINED-MEMORY %s
+! RUN: %flang --offload-arch=gfx90a --offload-device-only -fopenmp -emit-llvm -S %s -fatomic-fine-grained-memory -o -|FileCheck -check-prefix=FINE-GRAINED-MEMORY %s
 ! RUN: %flang_fc1 -emit-llvm -triple amdgcn-amd-amdhsa -fopenmp -fopenmp-is-device -fatomic-remote-memory %s -o -|FileCheck -check-prefix=REMOTE-MEMORY %s
+! RUN: %flang --offload-arch=gfx90a --offload-device-only -fopenmp -emit-llvm -S %s -fatomic-remote-memory -o -|FileCheck -check-prefix=REMOTE-MEMORY %s
 program test
     implicit none
     integer :: A, threads

@anchuraj anchuraj requested a review from mjklemm August 20, 2025 22:16
@llvmbot
Copy link
Member

llvmbot commented Aug 20, 2025

@llvm/pr-subscribers-clang

Author: Anchu Rajendran S (anchuraj)

Changes

Fix enables options in non-fc1 mode. The changes, #151579, #150860 enabled it only in fc1 mode.


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

2 Files Affected:

  • (modified) clang/lib/Driver/ToolChains/Flang.cpp (+8-1)
  • (modified) flang/test/Driver/atomic-control-options.f90 (+4)
diff --git a/clang/lib/Driver/ToolChains/Flang.cpp b/clang/lib/Driver/ToolChains/Flang.cpp
index 65391033c2b9f..1535f4cebf436 100644
--- a/clang/lib/Driver/ToolChains/Flang.cpp
+++ b/clang/lib/Driver/ToolChains/Flang.cpp
@@ -534,7 +534,14 @@ void Flang::addTargetOptions(const ArgList &Args,
   }
 
   Args.addAllArgs(CmdArgs,
-                  {options::OPT_fverbose_asm, options::OPT_fno_verbose_asm});
+                  {options::OPT_fverbose_asm, options::OPT_fno_verbose_asm,
+                   options::OPT_fatomic_ignore_denormal_mode,
+                   options::OPT_fno_atomic_ignore_denormal_mode,
+                   options::OPT_fatomic_fine_grained_memory,
+                   options::OPT_fno_atomic_fine_grained_memory,
+                   options::OPT_fatomic_remote_memory,
+                   options::OPT_fno_atomic_remote_memory,
+                   options::OPT_munsafe_fp_atomics});
 }
 
 void Flang::addOffloadOptions(Compilation &C, const InputInfoList &Inputs,
diff --git a/flang/test/Driver/atomic-control-options.f90 b/flang/test/Driver/atomic-control-options.f90
index cb382f96a9d5f..3251896033c25 100644
--- a/flang/test/Driver/atomic-control-options.f90
+++ b/flang/test/Driver/atomic-control-options.f90
@@ -1,8 +1,12 @@
 ! REQUIRES: amdgpu-registered-target
 ! RUN: %flang_fc1 -emit-llvm -triple amdgcn-amd-amdhsa -fopenmp -fopenmp-is-device -munsafe-fp-atomics %s -o -|FileCheck -check-prefix=UNSAFE-FP-ATOMICS %s
+! RUN: %flang --offload-arch=gfx90a --offload-device-only -fopenmp -emit-llvm -S %s -munsafe-fp-atomics -o -|FileCheck -check-prefix=UNSAFE-FP-ATOMICS %s
 ! RUN: %flang_fc1 -emit-llvm -triple amdgcn-amd-amdhsa -fopenmp -fopenmp-is-device -fatomic-ignore-denormal-mode %s -o -|FileCheck -check-prefix=IGNORE-DENORMAL-MODE %s
+! RUN: %flang --offload-arch=gfx90a --offload-device-only -fopenmp -emit-llvm -S %s -fatomic-ignore-denormal-mode -o -|FileCheck -check-prefix=IGNORE-DENORMAL-MODE %s
 ! RUN: %flang_fc1 -emit-llvm -triple amdgcn-amd-amdhsa -fopenmp -fopenmp-is-device -fatomic-fine-grained-memory %s -o -|FileCheck -check-prefix=FINE-GRAINED-MEMORY %s
+! RUN: %flang --offload-arch=gfx90a --offload-device-only -fopenmp -emit-llvm -S %s -fatomic-fine-grained-memory -o -|FileCheck -check-prefix=FINE-GRAINED-MEMORY %s
 ! RUN: %flang_fc1 -emit-llvm -triple amdgcn-amd-amdhsa -fopenmp -fopenmp-is-device -fatomic-remote-memory %s -o -|FileCheck -check-prefix=REMOTE-MEMORY %s
+! RUN: %flang --offload-arch=gfx90a --offload-device-only -fopenmp -emit-llvm -S %s -fatomic-remote-memory -o -|FileCheck -check-prefix=REMOTE-MEMORY %s
 program test
     implicit none
     integer :: A, threads

@anchuraj anchuraj force-pushed the atomic-control-fix branch from c04807b to f8a77d7 Compare August 20, 2025 22:39
Copy link
Contributor

@mjklemm mjklemm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@tarunprabhu tarunprabhu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks

@anchuraj anchuraj merged commit bce9b6d into llvm:main Aug 21, 2025
9 checks passed
@DanielCChen
Copy link
Contributor

DanielCChen commented Aug 29, 2025

@anchuraj
The addition of the offload testing in atomic-control-options.f90 broke our AIX build.

Our AIX build has the default LLVM_TARGETS_TO_BUILD setting which registered all targets so we can have the extended test coverage.

The reason of the failure is that the offload option triggered the call to addClangTargetOptions. On AIX, we passed down a few -f options that are supported by Clang only in that function. So the Flang driver complains

error: unknown argument: '-fxl-pragma-pack'
error: unknown argument: '-fno-sized-deallocation'
error: unknown argument: '-fno-err-pragma-mc-func-aix'

Would it be possible to split this test case into two; one for the host and one for the offload?
The offload one can be disabled for AIX, so we can still testing to host one to prevent any regressions.

@anchuraj
Copy link
Contributor Author

@anchuraj The addition of the offload testing in atomic-control-options.f90 broke our AIX build.

Our AIX build has the default LLVM_TARGETS_TO_BUILD setting which registered all targets so we can have the extended test coverage.

The reason of the failure is that the offload option triggered the call to addClangTargetOptions. On AIX, we passed down a few -f options that are supported by Clang only in that function. So the Flang driver complains

error: unknown argument: '-fxl-pragma-pack'
error: unknown argument: '-fno-sized-deallocation'
error: unknown argument: '-fno-err-pragma-mc-func-aix'

Would it be possible to split this test case into two; one for the host and one for the offload? The offload one can be disabled for AIX, so we can still testing to host one to prevent any regressions.
@DanielCChen the testcase now tests only device-only option and can be disabled if you would like. Let me know if I need to update any.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' clang Clang issues not falling into any other category flang:driver flang Flang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants