[flang][OpenMP] Don't allow namelist variables with threadprivate #130957
Conversation
While the standard doesn't explicitly mention namelist variables in this case, it does prohibit privatization of namelist variables. Flang decided to also prohibit namelist variables from appearing in a reduction clause. Variables that are part of a namelist are linked to I/O mechanisms, and their memory association and initialization are managed by the compiler in a way that conflicts with the requirements of threadprivate variables. I propose we should add this restriction.
tblah
requested review from
kiranchandramohan,
mjklemm,
luporl,
kparzysz and
ohno-fj
llvmbot
added
flang
|
@llvm/pr-subscribers-flang-openmp Author: Tom Eccles (tblah) ChangesWhile the standard doesn't explicitly mention namelist variables in this case, it does prohibit privatization of namelist variables. Flang decided to also prohibit namelist variables from appearing in a reduction clause. Variables that are part of a namelist are linked to I/O mechanisms, and their memory association and initialization are managed by the compiler in a way that conflicts with the requirements of threadprivate variables. I propose we should add this restriction. Fixes #112538 Full diff: https://github.com/llvm/llvm-project/pull/130957.diff 2 Files Affected:
diff --git a/flang/lib/Semantics/check-omp-structure.cpp b/flang/lib/Semantics/check-omp-structure.cpp
index 5fcebdca0bc5f..e98c9c3091cb4 100644
--- a/flang/lib/Semantics/check-omp-structure.cpp
+++ b/flang/lib/Semantics/check-omp-structure.cpp
@@ -1516,6 +1516,10 @@ void OmpStructureChecker::CheckThreadprivateOrDeclareTargetVar(
"A variable in a %s directive cannot appear in an "
"EQUIVALENCE statement"_err_en_US,
ContextDirectiveAsFortran());
+ } else if (name->symbol->test(Symbol::Flag::InNamelist)) {
+ context_.Say(name->source,
+ "A variable in a %s directive cannot appear in a NAMELIST"_err_en_US,
+ ContextDirectiveAsFortran());
} else if (name->symbol->test(Symbol::Flag::OmpThreadprivate) &&
GetContext().directive ==
llvm::omp::Directive::OMPD_declare_target) {
@@ -1571,6 +1575,11 @@ void OmpStructureChecker::CheckThreadprivateOrDeclareTargetVar(
ContextDirectiveAsFortran(), obj->name(),
name.symbol->name());
}
+ if (obj->test(Symbol::Flag::InNamelist)) {
+ context_.Say(name.source,
+ "A variable in a %s directive cannot appear in a NAMELIST"_err_en_US,
+ ContextDirectiveAsFortran());
+ }
}
}
}
diff --git a/flang/test/Semantics/OpenMP/threadprivate09.f90 b/flang/test/Semantics/OpenMP/threadprivate09.f90
new file mode 100644
index 0000000000000..02236d19d9d01
--- /dev/null
+++ b/flang/test/Semantics/OpenMP/threadprivate09.f90
@@ -0,0 +1,12 @@
+! RUN: %python %S/../test_errors.py %s %flang_fc1 %openmp_flags
+
+! Check that namelist variables cannot be used with threadprivate
+
+module m
+ integer :: nam1
+ common /com1/nam1
+ namelist /nam/nam1
+
+ !ERROR: A variable in a THREADPRIVATE directive cannot appear in a NAMELIST
+ !$omp threadprivate(/com1/)
+end
|
|
@llvm/pr-subscribers-flang-semantics Author: Tom Eccles (tblah) ChangesWhile the standard doesn't explicitly mention namelist variables in this case, it does prohibit privatization of namelist variables. Flang decided to also prohibit namelist variables from appearing in a reduction clause. Variables that are part of a namelist are linked to I/O mechanisms, and their memory association and initialization are managed by the compiler in a way that conflicts with the requirements of threadprivate variables. I propose we should add this restriction. Fixes #112538 Full diff: https://github.com/llvm/llvm-project/pull/130957.diff 2 Files Affected:
diff --git a/flang/lib/Semantics/check-omp-structure.cpp b/flang/lib/Semantics/check-omp-structure.cpp
index 5fcebdca0bc5f..e98c9c3091cb4 100644
--- a/flang/lib/Semantics/check-omp-structure.cpp
+++ b/flang/lib/Semantics/check-omp-structure.cpp
@@ -1516,6 +1516,10 @@ void OmpStructureChecker::CheckThreadprivateOrDeclareTargetVar(
"A variable in a %s directive cannot appear in an "
"EQUIVALENCE statement"_err_en_US,
ContextDirectiveAsFortran());
+ } else if (name->symbol->test(Symbol::Flag::InNamelist)) {
+ context_.Say(name->source,
+ "A variable in a %s directive cannot appear in a NAMELIST"_err_en_US,
+ ContextDirectiveAsFortran());
} else if (name->symbol->test(Symbol::Flag::OmpThreadprivate) &&
GetContext().directive ==
llvm::omp::Directive::OMPD_declare_target) {
@@ -1571,6 +1575,11 @@ void OmpStructureChecker::CheckThreadprivateOrDeclareTargetVar(
ContextDirectiveAsFortran(), obj->name(),
name.symbol->name());
}
+ if (obj->test(Symbol::Flag::InNamelist)) {
+ context_.Say(name.source,
+ "A variable in a %s directive cannot appear in a NAMELIST"_err_en_US,
+ ContextDirectiveAsFortran());
+ }
}
}
}
diff --git a/flang/test/Semantics/OpenMP/threadprivate09.f90 b/flang/test/Semantics/OpenMP/threadprivate09.f90
new file mode 100644
index 0000000000000..02236d19d9d01
--- /dev/null
+++ b/flang/test/Semantics/OpenMP/threadprivate09.f90
@@ -0,0 +1,12 @@
+! RUN: %python %S/../test_errors.py %s %flang_fc1 %openmp_flags
+
+! Check that namelist variables cannot be used with threadprivate
+
+module m
+ integer :: nam1
+ common /com1/nam1
+ namelist /nam/nam1
+
+ !ERROR: A variable in a THREADPRIVATE directive cannot appear in a NAMELIST
+ !$omp threadprivate(/com1/)
+end
|
|
If it is not in the standard, could you comment on what other compilers (gfortran, ifx, nvfortran/armflang) do? You can use godbolt for it. The function you are making this change is called |
Using the original fujitsu test (see #112538), nvfortran complains that nam1 and nam2 are not threadprivate (they are inside of a threadprivate common block so I don't think that is correct). GFortran does compile it (and produces a correct result). ifx and ifort also allow the code.
I'm still unsure why CI is failing and I can't reproduce the failure on my machine. Hopefully it will fix after I push a change. 🤞 |
|
With #131888 we can build the original test and it produces the correct results, however in my opinion it still doesn't make sense to mix namelists with threadprivate. It is up to you @kiranchandramohan |
|
I was initially in favor of restricting this, but a few people I work with and are closer to OpenMP disagreed and think that if the OpenMP standard did restrict the private clause case but told nothing about threadprivate, it is because the threadprivate case should work. So I would be OK supporting this by detecting that there is a threadprivate in the namelist and generating a local namelist descriptor near the IO statement in that case. |
While the standard doesn't explicitly mention namelist variables in this case, it does prohibit privatization of namelist variables. Flang decided to also prohibit namelist variables from appearing in a reduction clause.
Variables that are part of a namelist are linked to I/O mechanisms, and their memory association and initialization are managed by the compiler in a way that conflicts with the requirements of threadprivate variables. I propose we should add this restriction.