[flang][Semantics][OpenMP] don't reduce variables in namelist #110671
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is allowed by the OpenMP and F23 standards. But variables in a namelist are not allowed in OpenMP privatisation. I suspect this was an oversight. If we allow this we run into problems masking the original symbol with the symbol for the reduction variable when the variable is accessed via a namelist initialised as a global variable. See llvm#101907. One solution for this would be to force the namelist to always be initilized inside of the block in which it is used (therefore using the correct mapping for the reduction variable), but this could make some production applications slow. I tentatively think it is probably better to disallow a (perhaps mistaken) edge case of the standards with (I think) little practical use, than to make real applications slow in order to make this work. If reviewers would rather keep to the letter of the standard, see llvm#109303 which implements the alternative solution. I'm open to either path forward. Fixes llvm#101907
tblah
requested review from
DavidTruby,
ergawy,
skatrak,
mjklemm,
jeanPerier,
pawosm-arm,
luporl,
vdonaldson and
ohno-fj
llvmbot
added
flang
|
@llvm/pr-subscribers-flang-openmp Author: Tom Eccles (tblah) ChangesThis is allowed by the OpenMP and F23 standards. But variables in a namelist are not allowed in OpenMP privatisation. I suspect this was an oversight. If we allow this we run into problems masking the original symbol with the symbol for the reduction variable when the variable is accessed via a namelist initialised as a global variable. See #101907. One solution for this would be to force the namelist to always be initilized inside of the block in which it is used (therefore using the correct mapping for the reduction variable), but this could make some production applications slow. I tentatively think it is probably better to disallow a (perhaps mistaken) edge case of the standards with (I think) little practical use, than to make real applications slow in order to make this work. If reviewers would rather keep to the letter of the standard, see #109303 which implements the alternative solution. I'm open to either path forward. Fixes #101907 Full diff: https://github.com/llvm/llvm-project/pull/110671.diff 5 Files Affected:
diff --git a/flang/lib/Semantics/check-namelist.cpp b/flang/lib/Semantics/check-namelist.cpp
index b000e1771755ea..c2804c5d874e9c 100644
--- a/flang/lib/Semantics/check-namelist.cpp
+++ b/flang/lib/Semantics/check-namelist.cpp
@@ -34,4 +34,19 @@ void NamelistChecker::Leave(const parser::NamelistStmt &nmlStmt) {
}
}
+void NamelistChecker::Leave(const parser::LocalitySpec::Reduce &x) {
+ for (const parser::Name &name : std::get<std::list<parser::Name>>(x.t)) {
+ Symbol *sym{name.symbol};
+ // This is not disallowed by the standard, but would be difficult to
+ // support. This has to go here not with the other checks for locality specs
+ // in resolve-names.cpp so that it is done after the InNamelist flag is
+ // applied.
+ if (sym && sym->GetUltimate().test(Symbol::Flag::InNamelist)) {
+ context_.Say(name.source,
+ "NAMELIST variable '%s' not allowed in a REDUCE locality-spec"_err_en_US,
+ name.ToString());
+ }
+ }
+}
+
} // namespace Fortran::semantics
diff --git a/flang/lib/Semantics/check-namelist.h b/flang/lib/Semantics/check-namelist.h
index a23b7d717b022b..c51beb2cf25ae1 100644
--- a/flang/lib/Semantics/check-namelist.h
+++ b/flang/lib/Semantics/check-namelist.h
@@ -17,6 +17,7 @@ class NamelistChecker : public virtual BaseChecker {
public:
NamelistChecker(SemanticsContext &context) : context_{context} {}
void Leave(const parser::NamelistStmt &);
+ void Leave(const parser::LocalitySpec::Reduce &);
private:
SemanticsContext &context_;
diff --git a/flang/lib/Semantics/resolve-directives.cpp b/flang/lib/Semantics/resolve-directives.cpp
index 45fe17c0122c09..23a1ecbd2842d5 100644
--- a/flang/lib/Semantics/resolve-directives.cpp
+++ b/flang/lib/Semantics/resolve-directives.cpp
@@ -2384,6 +2384,21 @@ void OmpAttributeVisitor::ResolveOmpObject(
GetContext().directive)
.str()));
}
+ if (ompFlag == Symbol::Flag::OmpReduction) {
+ const Symbol &ultimateSymbol{symbol->GetUltimate()};
+ // Using variables inside of a namelist in OpenMP reductions
+ // is allowed by the standard, but is not allowed for
+ // privatisation. This looks like an oversight. If the
+ // namelist is hoisted to a global, we cannot apply the
+ // mapping for the reduction variable: resulting in incorrect
+ // results. Disabling this hoisting could make some real
+ // production code go slower. See discussion in #109303
+ if (ultimateSymbol.test(Symbol::Flag::InNamelist)) {
+ context_.Say(name->source,
+ "Variable '%s' in NAMELIST cannot be in a REDUCTION clause"_err_en_US,
+ name->ToString());
+ }
+ }
if (GetContext().directive ==
llvm::omp::Directive::OMPD_target_data) {
checkExclusivelists(symbol, Symbol::Flag::OmpUseDevicePtr,
diff --git a/flang/test/Semantics/OpenMP/reduction-namelist.f90 b/flang/test/Semantics/OpenMP/reduction-namelist.f90
new file mode 100644
index 00000000000000..edae3d7aef2c5d
--- /dev/null
+++ b/flang/test/Semantics/OpenMP/reduction-namelist.f90
@@ -0,0 +1,46 @@
+! RUN: %python %S/../test_errors.py %s %flang -fopenmp
+! This is not actually disallowed by the OpenMP standard, but it is not allowed
+! for privatisation - this seems like an oversight.
+
+module test
+ integer :: a, b, c
+ namelist /nlist1/ a, b
+end module
+
+program omp_reduction
+ use test
+
+ integer :: p(10) ,q(10)
+ namelist /nlist2/ c, d
+
+ a = 5
+ b = 10
+ c = 100
+
+ !ERROR: Variable 'd' in NAMELIST cannot be in a REDUCTION clause
+ !ERROR: Variable 'a' in NAMELIST cannot be in a REDUCTION clause
+ !$omp parallel reduction(+:d) reduction(+:a)
+ d = a + b
+ a = d
+ !$omp end parallel
+
+ call sb()
+
+ contains
+ subroutine sb()
+ namelist /nlist3/ p, q
+
+ !ERROR: Variable 'p' in NAMELIST cannot be in a REDUCTION clause
+ !ERROR: Variable 'q' in NAMELIST cannot be in a REDUCTION clause
+ !$omp parallel reduction(+:p) reduction(+:q)
+ p = c * b
+ q = p * d
+ !$omp end parallel
+
+ write(*, nlist1)
+ write(*, nlist2)
+ write(*, nlist3)
+
+ end subroutine
+
+end program omp_reduction
diff --git a/flang/test/Semantics/resolve123.f90 b/flang/test/Semantics/resolve123.f90
index 1b2c4613f2fefa..8f7c3acd443115 100644
--- a/flang/test/Semantics/resolve123.f90
+++ b/flang/test/Semantics/resolve123.f90
@@ -77,3 +77,13 @@ subroutine s9()
do concurrent(i=1:5) reduce(+:i)
end do
end subroutine s9
+
+subroutine s10()
+! Cannot have variable inside of a NAMELIST in a REDUCE locality spec
+ integer :: k
+ namelist /nlist1/ k
+!ERROR: NAMELIST variable 'k' not allowed in a REDUCE locality-spec
+ do concurrent(i=1:5) reduce(+:k)
+ k = k + i
+ end do
+end subroutine s10
|
|
@llvm/pr-subscribers-flang-semantics Author: Tom Eccles (tblah) ChangesThis is allowed by the OpenMP and F23 standards. But variables in a namelist are not allowed in OpenMP privatisation. I suspect this was an oversight. If we allow this we run into problems masking the original symbol with the symbol for the reduction variable when the variable is accessed via a namelist initialised as a global variable. See #101907. One solution for this would be to force the namelist to always be initilized inside of the block in which it is used (therefore using the correct mapping for the reduction variable), but this could make some production applications slow. I tentatively think it is probably better to disallow a (perhaps mistaken) edge case of the standards with (I think) little practical use, than to make real applications slow in order to make this work. If reviewers would rather keep to the letter of the standard, see #109303 which implements the alternative solution. I'm open to either path forward. Fixes #101907 Full diff: https://github.com/llvm/llvm-project/pull/110671.diff 5 Files Affected:
diff --git a/flang/lib/Semantics/check-namelist.cpp b/flang/lib/Semantics/check-namelist.cpp
index b000e1771755ea..c2804c5d874e9c 100644
--- a/flang/lib/Semantics/check-namelist.cpp
+++ b/flang/lib/Semantics/check-namelist.cpp
@@ -34,4 +34,19 @@ void NamelistChecker::Leave(const parser::NamelistStmt &nmlStmt) {
}
}
+void NamelistChecker::Leave(const parser::LocalitySpec::Reduce &x) {
+ for (const parser::Name &name : std::get<std::list<parser::Name>>(x.t)) {
+ Symbol *sym{name.symbol};
+ // This is not disallowed by the standard, but would be difficult to
+ // support. This has to go here not with the other checks for locality specs
+ // in resolve-names.cpp so that it is done after the InNamelist flag is
+ // applied.
+ if (sym && sym->GetUltimate().test(Symbol::Flag::InNamelist)) {
+ context_.Say(name.source,
+ "NAMELIST variable '%s' not allowed in a REDUCE locality-spec"_err_en_US,
+ name.ToString());
+ }
+ }
+}
+
} // namespace Fortran::semantics
diff --git a/flang/lib/Semantics/check-namelist.h b/flang/lib/Semantics/check-namelist.h
index a23b7d717b022b..c51beb2cf25ae1 100644
--- a/flang/lib/Semantics/check-namelist.h
+++ b/flang/lib/Semantics/check-namelist.h
@@ -17,6 +17,7 @@ class NamelistChecker : public virtual BaseChecker {
public:
NamelistChecker(SemanticsContext &context) : context_{context} {}
void Leave(const parser::NamelistStmt &);
+ void Leave(const parser::LocalitySpec::Reduce &);
private:
SemanticsContext &context_;
diff --git a/flang/lib/Semantics/resolve-directives.cpp b/flang/lib/Semantics/resolve-directives.cpp
index 45fe17c0122c09..23a1ecbd2842d5 100644
--- a/flang/lib/Semantics/resolve-directives.cpp
+++ b/flang/lib/Semantics/resolve-directives.cpp
@@ -2384,6 +2384,21 @@ void OmpAttributeVisitor::ResolveOmpObject(
GetContext().directive)
.str()));
}
+ if (ompFlag == Symbol::Flag::OmpReduction) {
+ const Symbol &ultimateSymbol{symbol->GetUltimate()};
+ // Using variables inside of a namelist in OpenMP reductions
+ // is allowed by the standard, but is not allowed for
+ // privatisation. This looks like an oversight. If the
+ // namelist is hoisted to a global, we cannot apply the
+ // mapping for the reduction variable: resulting in incorrect
+ // results. Disabling this hoisting could make some real
+ // production code go slower. See discussion in #109303
+ if (ultimateSymbol.test(Symbol::Flag::InNamelist)) {
+ context_.Say(name->source,
+ "Variable '%s' in NAMELIST cannot be in a REDUCTION clause"_err_en_US,
+ name->ToString());
+ }
+ }
if (GetContext().directive ==
llvm::omp::Directive::OMPD_target_data) {
checkExclusivelists(symbol, Symbol::Flag::OmpUseDevicePtr,
diff --git a/flang/test/Semantics/OpenMP/reduction-namelist.f90 b/flang/test/Semantics/OpenMP/reduction-namelist.f90
new file mode 100644
index 00000000000000..edae3d7aef2c5d
--- /dev/null
+++ b/flang/test/Semantics/OpenMP/reduction-namelist.f90
@@ -0,0 +1,46 @@
+! RUN: %python %S/../test_errors.py %s %flang -fopenmp
+! This is not actually disallowed by the OpenMP standard, but it is not allowed
+! for privatisation - this seems like an oversight.
+
+module test
+ integer :: a, b, c
+ namelist /nlist1/ a, b
+end module
+
+program omp_reduction
+ use test
+
+ integer :: p(10) ,q(10)
+ namelist /nlist2/ c, d
+
+ a = 5
+ b = 10
+ c = 100
+
+ !ERROR: Variable 'd' in NAMELIST cannot be in a REDUCTION clause
+ !ERROR: Variable 'a' in NAMELIST cannot be in a REDUCTION clause
+ !$omp parallel reduction(+:d) reduction(+:a)
+ d = a + b
+ a = d
+ !$omp end parallel
+
+ call sb()
+
+ contains
+ subroutine sb()
+ namelist /nlist3/ p, q
+
+ !ERROR: Variable 'p' in NAMELIST cannot be in a REDUCTION clause
+ !ERROR: Variable 'q' in NAMELIST cannot be in a REDUCTION clause
+ !$omp parallel reduction(+:p) reduction(+:q)
+ p = c * b
+ q = p * d
+ !$omp end parallel
+
+ write(*, nlist1)
+ write(*, nlist2)
+ write(*, nlist3)
+
+ end subroutine
+
+end program omp_reduction
diff --git a/flang/test/Semantics/resolve123.f90 b/flang/test/Semantics/resolve123.f90
index 1b2c4613f2fefa..8f7c3acd443115 100644
--- a/flang/test/Semantics/resolve123.f90
+++ b/flang/test/Semantics/resolve123.f90
@@ -77,3 +77,13 @@ subroutine s9()
do concurrent(i=1:5) reduce(+:i)
end do
end subroutine s9
+
+subroutine s10()
+! Cannot have variable inside of a NAMELIST in a REDUCE locality spec
+ integer :: k
+ namelist /nlist1/ k
+!ERROR: NAMELIST variable 'k' not allowed in a REDUCE locality-spec
+ do concurrent(i=1:5) reduce(+:k)
+ k = k + i
+ end do
+end subroutine s10
|
|
Thanks for addressing this issue - looks ok to me. |
jeanPerier
approved these changes
Oct 1, 2024
I agree to that sentiment. |
Sterling-Augustine
pushed a commit
to Sterling-Augustine/llvm-project
that referenced
this pull request
Oct 3, 2024
…10671) This is allowed by the OpenMP and F23 standards. But variables in a namelist are not allowed in OpenMP privatisation. I suspect this was an oversight. If we allow this we run into problems masking the original symbol with the symbol for the reduction variable when the variable is accessed via a namelist initialised as a global variable. See llvm#101907. One solution for this would be to force the namelist to always be initilized inside of the block in which it is used (therefore using the correct mapping for the reduction variable), but this could make some production applications slow. I tentatively think it is probably better to disallow a (perhaps mistaken) edge case of the standards with (I think) little practical use, than to make real applications slow in order to make this work. If reviewers would rather keep to the letter of the standard, see llvm#109303 which implements the alternative solution. I'm open to either path forward. Fixes llvm#101907
tblah
added a commit
to tblah/llvm-project
that referenced
this pull request
Mar 11, 2025
The standard prohibits privatising namelist variables. We also decided in llvm#110671 to prohibit reductions of namelist variables. This commit prevents this rule from being circumvented through the use of equivalence statements. Fixes llvm#122824
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is allowed by the OpenMP and F23 standards. But variables in a namelist are not allowed in OpenMP privatisation. I suspect this was an oversight.
If we allow this we run into problems masking the original symbol with the symbol for the reduction variable when the variable is accessed via a namelist initialised as a global variable. See #101907. One solution for this would be to force the namelist to always be initilized inside of the block in which it is used (therefore using the correct mapping for the reduction variable), but this could make some production applications slow.
I tentatively think it is probably better to disallow a (perhaps mistaken) edge case of the standards with (I think) little practical use, than to make real applications slow in order to make this work. If reviewers would rather keep to the letter of the standard, see #109303 which implements the alternative solution. I'm open to either path forward.
Fixes #101907