[flang][semantics][OpenMP] no privatisation of stmt functions #106550

tblah · 2024-08-29T12:55:59Z

OpenMP prohibits privatisation of variables that appear in expressions for statement functions.

This is a re-working of an old patch https://reviews.llvm.org/D93213 by @praveen-g-ctt.

The old patch couldn't be landed because of ordering concerns. Statement functions are rewritten during parse tree rewriting, but this was done after resolve-directives and so some array expressions were incorrectly identified as statement functions. For this reason I have opted to re-order the semantics driver so that resolve-directives is run after parse tree rewriting.

Closes #54677

llvmbot · 2024-08-29T12:56:31Z

@llvm/pr-subscribers-flang-fir-hlfir
@llvm/pr-subscribers-flang-openmp

@llvm/pr-subscribers-flang-semantics

Author: Tom Eccles (tblah)

Changes

OpenMP prohibits privatisation of variables that appear in expressions for statement functions.

This is a re-working of an old patch https://reviews.llvm.org/D93213 by @praveen-g-ctt.

The old patch couldn't be landed because of ordering concerns. Statement functions are rewritten during parse tree rewriting, but this was done after resolve-directives and so some array expressions were incorrectly identified as statement functions. For this reason I have opted to re-order the semantics driver so that resolve-directives is run after parse tree rewriting.

Closes #64677

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

3 Files Affected:

(modified) flang/lib/Semantics/resolve-directives.cpp (+11-3)
(modified) flang/lib/Semantics/semantics.cpp (+1-1)
(added) flang/test/Semantics/OpenMP/private03.f90 (+39)

diff --git a/flang/lib/Semantics/resolve-directives.cpp b/flang/lib/Semantics/resolve-directives.cpp
index 4aecb8b8e7b479..8e21dead15bf9c 100644
--- a/flang/lib/Semantics/resolve-directives.cpp
+++ b/flang/lib/Semantics/resolve-directives.cpp
@@ -717,7 +717,7 @@ class OmpAttributeVisitor : DirectiveAttributeVisitor<llvm::omp::Directive> {
   void CheckDataCopyingClause(
       const parser::Name &, const Symbol &, Symbol::Flag);
   void CheckAssocLoopLevel(std::int64_t level, const parser::OmpClause *clause);
-  void CheckObjectInNamelist(
+  void CheckPrivateDSAObject(
       const parser::Name &, const Symbol &, Symbol::Flag);
   void CheckSourceLabel(const parser::Label &);
   void CheckLabelContext(const parser::CharBlock, const parser::CharBlock,
@@ -2349,7 +2349,7 @@ void OmpAttributeVisitor::ResolveOmpObject(
                     CheckMultipleAppearances(*name, *symbol, ompFlag);
                   }
                   if (privateDataSharingAttributeFlags.test(ompFlag)) {
-                    CheckObjectInNamelist(*name, *symbol, ompFlag);
+                    CheckPrivateDSAObject(*name, *symbol, ompFlag);
                   }
 
                   if (ompFlag == Symbol::Flag::OmpAllocate) {
@@ -2706,7 +2706,7 @@ void OmpAttributeVisitor::CheckDataCopyingClause(
   }
 }
 
-void OmpAttributeVisitor::CheckObjectInNamelist(
+void OmpAttributeVisitor::CheckPrivateDSAObject(
     const parser::Name &name, const Symbol &symbol, Symbol::Flag ompFlag) {
   const auto &ultimateSymbol{symbol.GetUltimate()};
   llvm::StringRef clauseName{"PRIVATE"};
@@ -2721,6 +2721,14 @@ void OmpAttributeVisitor::CheckObjectInNamelist(
         "Variable '%s' in NAMELIST cannot be in a %s clause"_err_en_US,
         name.ToString(), clauseName.str());
   }
+
+  if (stmtFunctionExprSymbols_.find(ultimateSymbol) !=
+      stmtFunctionExprSymbols_.end()) {
+    context_.Say(name.source,
+        "Variable '%s' in STATEMENT FUNCTION expression cannot be in a "
+        "%s clause"_err_en_US,
+        name.ToString(), clauseName.str());
+  }
 }
 
 void OmpAttributeVisitor::CheckSourceLabel(const parser::Label &label) {
diff --git a/flang/lib/Semantics/semantics.cpp b/flang/lib/Semantics/semantics.cpp
index f7a277d1b414f6..2f1b9c139396a8 100644
--- a/flang/lib/Semantics/semantics.cpp
+++ b/flang/lib/Semantics/semantics.cpp
@@ -641,8 +641,8 @@ bool Semantics::Perform() {
       CanonicalizeAcc(context_.messages(), program_) &&
       CanonicalizeOmp(context_.messages(), program_) &&
       CanonicalizeCUDA(program_) &&
-      CanonicalizeDirectives(context_.messages(), program_) &&
       PerformStatementSemantics(context_, program_) &&
+      CanonicalizeDirectives(context_.messages(), program_) &&
       ModFileWriter{context_}
           .set_hermeticModuleFileOutput(hermeticModuleFileOutput_)
           .WriteAll();
diff --git a/flang/test/Semantics/OpenMP/private03.f90 b/flang/test/Semantics/OpenMP/private03.f90
new file mode 100644
index 00000000000000..3ce9cf81bb73a5
--- /dev/null
+++ b/flang/test/Semantics/OpenMP/private03.f90
@@ -0,0 +1,39 @@
+! RUN: %python %S/../test_errors.py %s %flang -fopenmp
+! OpenMP Version 4.5
+! Variables that appear in expressions for statement function definitions
+! may not appear in private, firstprivate or lastprivate clauses.
+
+subroutine stmt_function(temp)
+
+  integer :: i, p, q, r
+  real :: c, f, s, v, t(10)
+  real, intent(in) :: temp
+
+  c(temp) = p * (temp - q) / r
+  f(temp) = q + (temp * r/p)
+  v(temp) = c(temp) + f(temp)/2 - s
+
+  p = 5
+  q = 32
+  r = 9
+
+  !ERROR: Variable 'p' in STATEMENT FUNCTION expression cannot be in a PRIVATE clause
+  !$omp parallel private(p)
+  s = c(temp)
+  !$omp end parallel
+
+  !ERROR: Variable 's' in STATEMENT FUNCTION expression cannot be in a FIRSTPRIVATE clause
+  !$omp parallel firstprivate(s)
+  s = s + f(temp)
+  !$omp end parallel
+
+  !ERROR: Variable 's' in STATEMENT FUNCTION expression cannot be in a LASTPRIVATE clause
+  !$omp parallel do lastprivate(s, t)
+  do i = 1, 10
+  t(i) = v(temp) + i - s
+  end do
+  !$omp end parallel do
+
+  print *, t
+
+end subroutine stmt_function

klausler · 2024-08-29T13:54:28Z

flang/lib/Semantics/resolve-directives.cpp

+  if (stmtFunctionExprSymbols_.find(ultimateSymbol) !=
+      stmtFunctionExprSymbols_.end()) {
+    context_.Say(name.source,
+        "Variable '%s' in STATEMENT FUNCTION expression cannot be in a "


"statement function" isn't literal Fortran syntax, don't capitali[sz]e things that are not keywords.

And please don't break error message strings across multiple lines -- that makes it much more difficult to find them with grep.

Thanks for taking a look. I have removed the capitalization of "statement function" here.

I left the error message string across multiple lines because the next word in the string is %s anyway.

luporl · 2024-08-29T14:39:44Z

What about implicit privatisation, such as default(private) or implicitly firstprivate tasks?
OpenMP doesn't seem to allow these either, now that I had a closer look at the standard.

This means that #103390 would need to be reverted, even though gfortran and ifort perform implicit privatisation of variables in statement function expressions.

tblah · 2024-09-02T10:21:09Z

This means that #103390 would need to be reverted, even though gfortran and ifort perform implicit privatisation of variables in statement function expressions.

Good point about gfortran and ifort behavior. I'll bring it up on the OpenMP call this week.

tblah · 2024-09-09T10:43:32Z

An RFC to discuss whether to proceed with this: https://discourse.llvm.org/t/rfc-openmp-remove-support-for-privatisation-of-statement-function-variables/81138

@praveen-g-ctt

OpenMP prohibits privatisation of variables that appear in expressions for statement functions. This is a re-working of an old patch https://reviews.llvm.org/D93213 by @praveen-g-ctt. The old patch couldn't be landed because of ordering concerns. Statement functions are rewritten during parse tree rewriting, but this was done after resolve-directives and so some array expressions were incorrectly identified as statement functions. For this reason **I have opted to re-order the semantics driver so that resolve-directives is run after parse tree rewriting**. Closes llvm#54677 Co-authored-by: Praveen <praveen@compilertree.com>

tblah · 2024-10-02T10:04:00Z

The conclusion of the RFC was that we should move forward with this. Ping for review

luporl

OmpAttributeVisitor::Post(const parser::Name &name) and DataSharingProcessor::collectSymbols also need to be changed, to avoid implicit privatisation of stmt functions.
But this can be done in a separate patch.

luporl · 2024-10-02T16:44:38Z

flang/lib/Semantics/resolve-directives.cpp

+  if (stmtFunctionExprSymbols_.find(ultimateSymbol) !=
+      stmtFunctionExprSymbols_.end()) {
+    context_.Say(name.source,
+        "Variable '%s' in statement function expression cannot be in a "
+        "%s clause"_err_en_US,
+        name.ToString(), clauseName.str());
+  }


It would be better to change the name of this member function, to something like CheckObjectIsPrivatizable, now that it also checks for statement functions.

tblah · 2024-10-03T13:07:46Z

Thanks for reminding me. I lost track amongst all of the discussions.

Quite a bit of context changed since your initial commit so a straightforward revert would have been challenging. Please let me know if I missed anything.

luporl

LGTM, thanks for undoing the implicit privatization of statement functions. I agree that it is better than a revert at this point.

nit: the OmpFromStmtFunction flag should also be removed.

luporl · 2024-10-03T13:44:56Z

flang/lib/Semantics/resolve-directives.cpp

@@ -718,7 +718,7 @@ class OmpAttributeVisitor : DirectiveAttributeVisitor<llvm::omp::Directive> {
  void CheckDataCopyingClause(
      const parser::Name &, const Symbol &, Symbol::Flag);
  void CheckAssocLoopLevel(std::int64_t level, const parser::OmpClause *clause);
-  void CheckObjectInNamelistOrAssociate(
+  void CheckObjectIsPrivatisable(


I've noticed that sometimes privatisable (British English) is used and other times it's privatizable.
It would be nice to standardize one of the two in the code.
I suggest the latter, because that's how it's written in the OpenMP standard, that uses words such as privatization and privatized.

I agree we should use the American spelling.

@praveen-g-ctt

…06550) OpenMP prohibits privatisation of variables that appear in expressions for statement functions. This is a re-working of an old patch https://reviews.llvm.org/D93213 by @praveen-g-ctt. The old patch couldn't be landed because of ordering concerns. Statement functions are rewritten during parse tree rewriting, but this was done after resolve-directives and so some array expressions were incorrectly identified as statement functions. For this reason **I have opted to re-order the semantics driver so that resolve-directives is run after parse tree rewriting**. Closes llvm#54677 --------- Co-authored-by: Praveen <praveen@compilertree.com>

tblah requested review from DavidTruby, skatrak, kiranchandramohan, pawosm-arm, klausler and praveen-g-ctt August 29, 2024 12:55

llvmbot added flang Flang issues not falling into any other category flang:openmp flang:semantics labels Aug 29, 2024

tblah force-pushed the ecclescake/stmt_func_priv branch from 24c4455 to fb8b074 Compare August 29, 2024 12:57

klausler approved these changes Aug 29, 2024

View reviewed changes

tblah and others added 3 commits October 2, 2024 09:57

Don't capitalise 'statement function'

9a43a1d

Fix test

d3aa6da

tblah force-pushed the ecclescake/stmt_func_priv branch from 25f6a9f to d3aa6da Compare October 2, 2024 10:02

luporl reviewed Oct 2, 2024

View reviewed changes

Ensure that statement functions are not implicitly privatised

413e71c

llvmbot added the flang:fir-hlfir label Oct 3, 2024

luporl approved these changes Oct 3, 2024

View reviewed changes

Fix spelling and remove unused flag

99572e6

tblah merged commit c734d77 into llvm:main Oct 4, 2024
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[flang][semantics][OpenMP] no privatisation of stmt functions #106550

[flang][semantics][OpenMP] no privatisation of stmt functions #106550

tblah commented Aug 29, 2024 •

edited

Loading

llvmbot commented Aug 29, 2024 •

edited

Loading

klausler Aug 29, 2024

tblah Aug 29, 2024

luporl commented Aug 29, 2024

tblah commented Sep 2, 2024

tblah commented Sep 9, 2024

tblah commented Oct 2, 2024

luporl left a comment

luporl Oct 2, 2024

tblah commented Oct 3, 2024

luporl left a comment

luporl Oct 3, 2024

tblah Oct 3, 2024

[flang][semantics][OpenMP] no privatisation of stmt functions #106550

[flang][semantics][OpenMP] no privatisation of stmt functions #106550

Conversation

tblah commented Aug 29, 2024 • edited Loading

llvmbot commented Aug 29, 2024 • edited Loading

klausler Aug 29, 2024

Choose a reason for hiding this comment

tblah Aug 29, 2024

Choose a reason for hiding this comment

luporl commented Aug 29, 2024

tblah commented Sep 2, 2024

tblah commented Sep 9, 2024

tblah commented Oct 2, 2024

luporl left a comment

Choose a reason for hiding this comment

luporl Oct 2, 2024

Choose a reason for hiding this comment

tblah commented Oct 3, 2024

luporl left a comment

Choose a reason for hiding this comment

luporl Oct 3, 2024

Choose a reason for hiding this comment

tblah Oct 3, 2024

Choose a reason for hiding this comment

tblah commented Aug 29, 2024 •

edited

Loading

llvmbot commented Aug 29, 2024 •

edited

Loading