[Flang][OpenMP] Add -fopenmp-default-none command line flag #120287

mjklemm · 2024-12-17T18:53:02Z

This PR adds the -fopenmp-default-none command line flag to the Flang compiler. Similarly, to -fimplicit-none it provides error checking for OpenMP directives by behaving as if DEFAULT(NONE) was specified at all directives that support data-sharing attribute clauses.

llvmbot · 2024-12-17T18:53:39Z

@llvm/pr-subscribers-flang-semantics
@llvm/pr-subscribers-flang-driver

@llvm/pr-subscribers-clang-driver

Author: Michael Klemm (mjklemm)

Changes

This PR adds the -fopenmp-default-none command line flag to the Flang compiler. Similarly, to -fimplicit-none it provides error checking for OpenMP directives by behaving as if DEFAULT(NONE) was specified at all directives that support data-sharing attribute clauses.

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

7 Files Affected:

(modified) clang/include/clang/Driver/Options.td (+3)
(modified) clang/lib/Driver/ToolChains/Flang.cpp (+3)
(modified) flang/include/flang/Common/Fortran-features.h (+1-1)
(modified) flang/lib/Common/Fortran-features.cpp (+1)
(modified) flang/lib/Frontend/CompilerInvocation.cpp (+4)
(modified) flang/lib/Semantics/resolve-directives.cpp (+7-1)
(added) flang/test/Semantics/OpenMP/resolve07.f90 (+34)

diff --git a/clang/include/clang/Driver/Options.td b/clang/include/clang/Driver/Options.td
index bed2a56b003512..acbe93fe72a24e 100644
--- a/clang/include/clang/Driver/Options.td
+++ b/clang/include/clang/Driver/Options.td
@@ -3596,6 +3596,9 @@ def fopenmp : Flag<["-"], "fopenmp">, Group<f_Group>,
   Flags<[NoArgumentUnused]>,
   Visibility<[ClangOption, CC1Option, FlangOption, FC1Option]>,
   HelpText<"Parse OpenMP pragmas and generate parallel code.">;
+def fopenmp_default_none : Flag<["-"], "fopenmp-default-none">, Group<f_Group>,
+  Visibility<[FlangOption, FC1Option]>,
+  HelpText<"Add DEFAULT(NONE) to all OpenMP directives that allow data-sharing clauses.">;
 def fno_openmp : Flag<["-"], "fno-openmp">, Group<f_Group>,
   Visibility<[ClangOption, CC1Option, FlangOption, FC1Option]>,
   Flags<[NoArgumentUnused]>;
diff --git a/clang/lib/Driver/ToolChains/Flang.cpp b/clang/lib/Driver/ToolChains/Flang.cpp
index c98fdbd157bac8..521ae90e0bd164 100644
--- a/clang/lib/Driver/ToolChains/Flang.cpp
+++ b/clang/lib/Driver/ToolChains/Flang.cpp
@@ -855,6 +855,9 @@ void Flang::ConstructJob(Compilation &C, const JobAction &JA,
       // is experimental.
       D.Diag(diag::warn_openmp_experimental);
 
+      if (Args.hasArg((options::OPT_fopenmp_default_none)))
+        CmdArgs.push_back("-fopenmp-default-none");
+
       // FIXME: Clang supports a whole bunch more flags here.
       break;
     default:
diff --git a/flang/include/flang/Common/Fortran-features.h b/flang/include/flang/Common/Fortran-features.h
index b04f6117ae9656..218e05e53aee01 100644
--- a/flang/include/flang/Common/Fortran-features.h
+++ b/flang/include/flang/Common/Fortran-features.h
@@ -54,7 +54,7 @@ ENUM_CLASS(LanguageFeature, BackslashEscapes, OldDebugLines,
     PolymorphicActualAllocatableOrPointerToMonomorphicDummy, RelaxedPureDummy,
     UndefinableAsynchronousOrVolatileActual, AutomaticInMainProgram, PrintCptr,
     SavedLocalInSpecExpr, PrintNamelist, AssumedRankPassedToNonAssumedRank,
-    IgnoreIrrelevantAttributes)
+    IgnoreIrrelevantAttributes, OpenMPDefaultNone)
 
 // Portability and suspicious usage warnings
 ENUM_CLASS(UsageWarning, Portability, PointerToUndefinable,
diff --git a/flang/lib/Common/Fortran-features.cpp b/flang/lib/Common/Fortran-features.cpp
index c86432874b8d83..c6f818478b065c 100644
--- a/flang/lib/Common/Fortran-features.cpp
+++ b/flang/lib/Common/Fortran-features.cpp
@@ -17,6 +17,7 @@ LanguageFeatureControl::LanguageFeatureControl() {
   disable_.set(LanguageFeature::OldDebugLines);
   disable_.set(LanguageFeature::OpenACC);
   disable_.set(LanguageFeature::OpenMP);
+  disable_.set(LanguageFeature::OpenMPDefaultNone);
   disable_.set(LanguageFeature::CUDA); // !@cuf
   disable_.set(LanguageFeature::CudaManaged);
   disable_.set(LanguageFeature::CudaUnified);
diff --git a/flang/lib/Frontend/CompilerInvocation.cpp b/flang/lib/Frontend/CompilerInvocation.cpp
index 648b88e84051c2..648ad224a26f66 100644
--- a/flang/lib/Frontend/CompilerInvocation.cpp
+++ b/flang/lib/Frontend/CompilerInvocation.cpp
@@ -1023,6 +1023,10 @@ static bool parseOpenMPArgs(CompilerInvocation &res, llvm::opt::ArgList &args,
           res.getLangOpts().OpenMPVersion, diags)) {
     res.getLangOpts().OpenMPVersion = Version;
   }
+  if (args.hasArg(clang::driver::options::OPT_fopenmp_default_none)) {
+    res.getFrontendOpts().features.Enable(
+        Fortran::common::LanguageFeature::OpenMPDefaultNone);
+  }
   if (args.hasArg(clang::driver::options::OPT_fopenmp_force_usm)) {
     res.getLangOpts().OpenMPForceUSM = 1;
   }
diff --git a/flang/lib/Semantics/resolve-directives.cpp b/flang/lib/Semantics/resolve-directives.cpp
index 39478b58a9070d..9cefc36d987b15 100644
--- a/flang/lib/Semantics/resolve-directives.cpp
+++ b/flang/lib/Semantics/resolve-directives.cpp
@@ -2268,6 +2268,11 @@ void OmpAttributeVisitor::CreateImplicitSymbols(
 void OmpAttributeVisitor::Post(const parser::Name &name) {
   auto *symbol{name.symbol};
 
+  // if -fopenmp-default-none was given on the command line, act as if
+  // DEFAULT(NONE) was present at the directive.
+  bool HaveOpenMPDefaultNone = context_.languageFeatures().IsEnabled(
+      common::LanguageFeature::OpenMPDefaultNone);
+
   if (symbol && !dirContext_.empty() && GetContext().withinConstruct) {
     if (IsPrivatizable(symbol) && !IsObjectWithDSA(*symbol)) {
       // TODO: create a separate function to go through the rules for
@@ -2276,7 +2281,8 @@ void OmpAttributeVisitor::Post(const parser::Name &name) {
       if (Symbol * found{currScope().FindSymbol(name.source)}) {
         if (symbol != found) {
           name.symbol = found; // adjust the symbol within region
-        } else if (GetContext().defaultDSA == Symbol::Flag::OmpNone &&
+        } else if ((HaveOpenMPDefaultNone ||
+                       GetContext().defaultDSA == Symbol::Flag::OmpNone) &&
             !symbol->test(Symbol::Flag::OmpThreadprivate) &&
             // Exclude indices of sequential loops that are privatised in
             // the scope of the parallel region, and not in this scope.
diff --git a/flang/test/Semantics/OpenMP/resolve07.f90 b/flang/test/Semantics/OpenMP/resolve07.f90
new file mode 100644
index 00000000000000..4a898903fae431
--- /dev/null
+++ b/flang/test/Semantics/OpenMP/resolve07.f90
@@ -0,0 +1,34 @@
+! RUN: %python %S/../test_errors.py %s %flang -fopenmp -fopenmp-default-none
+
+
+! Test that -fopenmp-default-none shows the same errors as DEFAULT(NONE)
+subroutine default_none()
+  integer a(3)
+  integer, parameter :: D=10
+  A = 1
+  B = 2
+  !$omp parallel private(c)
+  !ERROR: The DEFAULT(NONE) clause requires that 'a' must be listed in a data-sharing attribute clause
+  A(1:2) = 3
+  !ERROR: The DEFAULT(NONE) clause requires that 'b' must be listed in a data-sharing attribute clause
+  B = 4
+  C = 5 + D
+  !$omp end parallel
+end subroutine default_none
+
+! Test that indices of sequential loops are privatised and hence do not error
+! for -fopenmp-default-none.
+subroutine default_none_seq_loop
+  integer :: i
+
+  !$omp parallel do
+  do i = 1, 10
+     do j = 1, 20
+    enddo
+  enddo
+end subroutine
+
+program mm
+  call default_none()
+  call default_none_seq_loop()
+end

tarunprabhu

On the whole, this looks reasonable.

tarunprabhu · 2024-12-17T19:44:26Z

clang/include/clang/Driver/Options.td

@@ -3596,6 +3596,9 @@ def fopenmp : Flag<["-"], "fopenmp">, Group<f_Group>,
  Flags<[NoArgumentUnused]>,
  Visibility<[ClangOption, CC1Option, FlangOption, FC1Option]>,
  HelpText<"Parse OpenMP pragmas and generate parallel code.">;
+def fopenmp_default_none : Flag<["-"], "fopenmp-default-none">, Group<f_Group>,
+  Visibility<[FlangOption, FC1Option]>,


Do you mind moving this to later in this file? Maybe so it is before def fopenmp_use_tls. There is a loose ordering of options here with the "top-level" fopenmp options first and some more specific ones later. This file is already huge and somewhat disorganized, so it would be good to keep what organization there is.

Done. I have seen that there seems to be a bit of chaos (or: I did not the see the order :-)) in the order of the CLI options. Maybe it warrants an NFC PR that puts the options in some better order.

flang/lib/Semantics/resolve-directives.cpp

kiranchandramohan · 2024-12-17T19:50:58Z

flang/lib/Semantics/resolve-directives.cpp

@@ -2276,7 +2281,8 @@ void OmpAttributeVisitor::Post(const parser::Name &name) {
      if (Symbol * found{currScope().FindSymbol(name.source)}) {
        if (symbol != found) {
          name.symbol = found; // adjust the symbol within region
-        } else if (GetContext().defaultDSA == Symbol::Flag::OmpNone &&
+        } else if ((HaveOpenMPDefaultNone ||


Would HaveOpenMPDefaultNone be set for those constructs where data-sharing is not allowed as well?

As far as I understand the code, this is the place where all the handling of the implicit and explicit data-sharing checks are happening. So, this should happen for all directives that allow for having data-sharing attribute clauses.

I think this code used to be entered only if defaultDSA is set to None. And this setting is explicitly done in resolve-directives while going through the OmpDefaultClause in

llvm-project/flang/lib/Semantics/resolve-directives.cpp

Line 2019 in 4b56345

void OmpAttributeVisitor::Post(const parser::OmpDefaultClause &x) {

. Whereas the HaveOpenMPDefaultNone flag is set just based on the command line flag setting. We probably have to enable this flag only when datasharing clauses are supported.

To give an example where this will fire incorrectly.

subroutine sb1(arr, n) integer :: n integer :: arr(n) !$omp workshare arr = n !$omp end workshare end subroutine

This is exactly what this option is supposed to do. If I misread your comment, please bear with me and explain. For

subroutine sb1(arr, n) integer :: n integer :: arr(n) !$omp workshare arr = n !$omp end workshare end subroutine

The -fopenmp-default-none acts as if the developer has written this:

subroutine sb1(arr, n) integer :: n integer :: arr(n) !$omp workshare default(none) arr = n !$omp end workshare end subroutine

So, for both the error would be the same:

error: Semantic errors in /net/home/micha/projects/llvm/tmp/datasharing.f90 /net/home/micha/projects/llvm/tmp/datasharing.f90:16:5: error: The DEFAULT(NONE) clause requires that 'arr' must be listed in a data-sharing attribute clause arr = n ^^^ /net/home/micha/projects/llvm/tmp/datasharing.f90:16:11: error: The DEFAULT(NONE) clause requires that 'n' must be listed in a data-sharing attribute clause arr = n

I mean workshare does not support a data-sharing clause. I thought the intent of the patch was to only apply this flag to constructs where data-sharing clauses are allowed.

There is probably a bug here since resolve-directives happens before check-omp-structure.

Oh boy... Yes, I do not like these 20th century features. :-) Thanks for catching this!

kiranchandramohan · 2024-12-17T19:51:31Z

clang/include/clang/Driver/Options.td

+def fopenmp_default_none : Flag<["-"], "fopenmp-default-none">, Group<f_Group>,
+  Visibility<[FlangOption, FC1Option]>,
+  HelpText<"Add DEFAULT(NONE) to all OpenMP directives that allow data-sharing clauses.">;


This seems to be a useful flag. Is there a precedent in other compiler?

I'm not aware that GFortran or IFX would have that. And I always sort of was annoyed by that. :-)

DavidTruby

Would it be a lot of work to add this flag for clang as well? It seems like the behaviour would be equally useful there and I would prefer us to keep flags available for both compilers where it makes sense.
In terms of implementation it looks like there's already a clang-tidy pass for it so you could check what they are doing for inspiration?
If it's a lot of work we can add a TODO somewhere (I guess in a comment in the tablegen file) to remind us that we could conceptually enable this flag for clang too so as not to hold up this patch.

mjklemm · 2024-12-18T13:06:27Z

Would it be a lot of work to add this flag for clang as well? It seems like the behaviour would be equally useful there and I would prefer us to keep flags available for both compilers where it makes sense.

I will ask one of the Clang adepts in the team to provide a PR or extend this one. I think it should be fairly easy to do that there as well.

If it's a lot of work we can add a TODO somewhere (I guess in a comment in the tablegen file) to remind us that we could conceptually enable this flag for clang too so as not to hold up this patch.

Let me check this out and I'll either add the TODO or extent this PR (or have a second one for Clang)

kparzysz · 2024-12-18T13:47:38Z

Is NONE special enough to have a dedicated option, or could we have -fopenmp-default=<dsa> for any dsa?

mjklemm · 2024-12-18T17:08:00Z

Is NONE special enough to have a dedicated option, or could we have -fopenmp-default=<dsa> for any dsa?

I have thought about this, but that's going to be dangerous. I intended this option to be solely for detecting potential races through unintended sharing.

luporl

LG, except for the issue raised by @kiranchandramohan, to skip constructs that don't support data-sharing clauses.

luporl · 2024-12-18T18:54:00Z

clang/lib/Driver/ToolChains/Flang.cpp

@@ -855,6 +855,9 @@ void Flang::ConstructJob(Compilation &C, const JobAction &JA,
      // is experimental.
      D.Diag(diag::warn_openmp_experimental);

+      if (Args.hasArg((options::OPT_fopenmp_default_none)))


Suggested change

if (Args.hasArg((options::OPT_fopenmp_default_none)))

if (Args.hasArg(options::OPT_fopenmp_default_none))

nit: extra parentheses

luporl · 2024-12-18T19:00:42Z

flang/test/Semantics/OpenMP/resolve07.f90

+  call default_none()
+  call default_none_seq_loop()


It would be nice to add a test for a construct that doesn't support data-sharing clauses, such as workshare, to make sure no error is reported.

mjklemm added 2 commits December 17, 2024 18:43

Implement -fopenmp-default-none compiler flag

ef9b76a

Add test for -fopenmp-default-none

e5a71fc

mjklemm requested review from kiranchandramohan, luporl and klausler December 17, 2024 18:53

mjklemm self-assigned this Dec 17, 2024

llvmbot added clang clang:driver flang:driver flang flang:openmp flang:semantics labels Dec 17, 2024

mjklemm requested a review from kparzysz December 17, 2024 18:53

mjklemm requested review from skatrak, agozillon and tblah December 17, 2024 18:53

klausler removed their request for review December 17, 2024 18:54

kiranchandramohan requested review from DavidTruby and tarunprabhu December 17, 2024 19:36

tarunprabhu reviewed Dec 17, 2024

View reviewed changes

kiranchandramohan reviewed Dec 17, 2024

View reviewed changes

DavidTruby reviewed Dec 18, 2024

View reviewed changes

mjklemm added 2 commits December 18, 2024 13:53

Move -fopenmp-default-none to the end of OpenMP options

4dbad74

Rename variable

ffafbfd

luporl reviewed Dec 18, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Flang][OpenMP] Add -fopenmp-default-none command line flag #120287

[Flang][OpenMP] Add -fopenmp-default-none command line flag #120287

mjklemm commented Dec 17, 2024 •

edited

Loading

llvmbot commented Dec 17, 2024 •

edited

Loading

tarunprabhu left a comment

tarunprabhu Dec 17, 2024

mjklemm Dec 18, 2024

kiranchandramohan Dec 17, 2024

mjklemm Dec 18, 2024

kiranchandramohan Dec 18, 2024

mjklemm Dec 18, 2024

kiranchandramohan Dec 18, 2024

kiranchandramohan Dec 18, 2024

mjklemm Dec 18, 2024

kiranchandramohan Dec 17, 2024

mjklemm Dec 18, 2024 •

edited

Loading

DavidTruby left a comment

mjklemm commented Dec 18, 2024

kparzysz commented Dec 18, 2024

mjklemm commented Dec 18, 2024

luporl left a comment

luporl Dec 18, 2024

luporl Dec 18, 2024

	if (Args.hasArg((options::OPT_fopenmp_default_none)))
	if (Args.hasArg(options::OPT_fopenmp_default_none))

[Flang][OpenMP] Add -fopenmp-default-none command line flag #120287

Are you sure you want to change the base?

[Flang][OpenMP] Add -fopenmp-default-none command line flag #120287

Conversation

mjklemm commented Dec 17, 2024 • edited Loading

llvmbot commented Dec 17, 2024 • edited Loading

tarunprabhu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjklemm Dec 18, 2024 • edited Loading

Choose a reason for hiding this comment

DavidTruby left a comment

Choose a reason for hiding this comment

mjklemm commented Dec 18, 2024

kparzysz commented Dec 18, 2024

mjklemm commented Dec 18, 2024

luporl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjklemm commented Dec 17, 2024 •

edited

Loading

llvmbot commented Dec 17, 2024 •

edited

Loading

mjklemm Dec 18, 2024 •

edited

Loading