[flang][OpenMP]Add symbls omp_in, omp_out and omp_priv in DECLARE RED… #129908

Leporacanthicus · 2025-03-05T18:20:29Z

…UCTION

This patch allows better parsing of the reduction and initializer components, including supporting derived types in both those places.

There is more work needed here, but this is a definite improvement in what can be handled through parser and semantics.

Note that declare reduction is still not supported in lowering, so any attempt to compile DECLARE REDUCTION code will end with a TODO aka "Not yet implemented" abort in the compiler.

Note that this version of the code does not cover declaring multiple reductions using the same name with different types. This is will be fixed in a future patch. [This was also the case before this change].

One existing test modified to actually compile (as it didn't in the original form).

…UCTION This patch allows better parsing of the reduction and initializer components, including supporting derived types in both those places. There is more work needed here, but this is a definite improvement in what can be handled through parser and semantics. Note that declare reduction is still not supported in lowering, so any attempt to compile DECLARE REDUCTION code will end with a TODO aka "Not yet implemented" abort in the compiler. Note that this version of the code does not cover declaring multiple reductions using the same name with different types. This is will be fixed in a future patch. [This was also the case before this change]. One existing test modified to actually compile (as it didn't in the original form).

kparzysz

This is great!

kparzysz · 2025-03-05T18:35:51Z

flang/lib/Parser/unparse.cpp

+    if (const auto *assignment = std::get_if<AssignmentStmt>(&x.u))
+      Walk(assignment->t, "=");
+    else
+      Walk(x.u);


The convention in Parser is to use braces even with single statements.

flang/lib/Parser/unparse.cpp

flang/lib/Semantics/resolve-names.cpp

kiranchandramohan · 2025-03-06T08:26:01Z

flang/lib/Semantics/resolve-names.cpp

+    const parser::CharBlock ompVarNames[] = {
+        {"omp_in", 6}, {"omp_out", 7}, {"omp_priv", 8}};


Are these always present? Are they not bound to any names?

Braced init if possible.

As I understand it, these are the names that need to be present with the correct type for the reduction combiner and initializesr. The code here is to introduce T omp_in, omp_out, omp_priv where T is the type given in the list of types. Without doing this, code that does something like intializer(omp_priv%x = 1) will not compile.

Changed to braced initializer.

But the initializer clause itself is optional, so omp_priv is not guaranteed to be always there.
Also, now that I think about it, shouldn't handling of omp_priv be in bool Pre(const parser::OmpReductionInitializerProc &x) { ?

We could add that code into the OmpReductionInitializerProc, where the code would be something like initme(omp_priv) - for the case of omp_priv=7 or omp_priv%x=1, that is just a standard Fortran AssignmentStmt, which either would require an extra wrapper or additional code here to handle that case. Of course.

I guess to avoid generating a symbol that isn'g used, we could check if there is an initializer (relatively easy in terms of code, as there is only one valid clause allowed here, so it's either an initializer clause or the clauselist is empty. It does add a bit more complication to this code, and I'm not sure it's worth the extra complexity to avoid generating the omp_priv symbol some of the time.

Is the concern here that these symbols will somehow cause a problem elsewhere? Or simply that we shouldn't do things that aren't required in general. They are in a local scope, so won't be visible outside of the declaration of the reduction.

[In theory, the omp_in may also not be used, but without following what's in the Reduction specifier, which could be an arbitrary expression as far as I understand, it's even harder to know if that is or isn't used].

I think it will be good to comment on what you are trying to do here. Are you trying to create as many sets of symbols as there are in the typeList? And you chose to do it here because this is where the typeList is processed?

Also, if these symbols are not tied to names, what is the use of these symbols?

It will be good to not generate symbols that are not required.

There is one test, flang/test/Parser/OpenMP/metadirective-dirspec.f90 that has multiple types, I have some more comprehensive tests when more functionality has been implemented (being worked on now)

Multiple separate declarations with the same name (not supported in this version of the code).

When we store the type list in the symbol details, we need to test that those are indeed stored correctly.

Also need tests that cover multiple types on operators (+, *, etc).

@Leporacanthicus I am not following you here. Could you reply to my following questions?

flang/test/Parser/OpenMP/metadirective-dirspec.f90 : This is a dump parse tree test. Would you know whether the parser names omp_in, omp_out in this test are bound to a symbol? And if so, is it with the first type or the second type?

Are you ignoring the case of omp_orig here?

They are bound to something because otherwise we'd get an assertion that not all names have been resolved. If I was to guess, I'd say that they are most likely declared implicitly.

I assumed that since this patch worked, having symbols with the same name but different types was valid. I think we could declare them once with whatever type (integer for example) and avoid using core Fortran parse tree nodes to store the initialization/update that could trigger semantic errors (such as AssignmentStmt). The types of these symbols would then become irrelevant, but we'd have to type-check the update expression ourselves.

Completely missed that one. Partly because there's no test at all for that name...

Is it ok to add a separate patch for that, or do you want me to add this on this patch?

Well, I pushed a patch to fix it. There's still no test other than the one checking generated symbols. I will add one or two that uses omp_orig in the next set.

flang/lib/Semantics/resolve-names.cpp

llvmbot · 2025-03-06T12:23:03Z

@llvm/pr-subscribers-flang-semantics

@llvm/pr-subscribers-flang-openmp

Author: Mats Petersson (Leporacanthicus)

Changes

…UCTION

This patch allows better parsing of the reduction and initializer components, including supporting derived types in both those places.

There is more work needed here, but this is a definite improvement in what can be handled through parser and semantics.

Note that declare reduction is still not supported in lowering, so any attempt to compile DECLARE REDUCTION code will end with a TODO aka "Not yet implemented" abort in the compiler.

Note that this version of the code does not cover declaring multiple reductions using the same name with different types. This is will be fixed in a future patch. [This was also the case before this change].

One existing test modified to actually compile (as it didn't in the original form).

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

8 Files Affected:

(modified) flang/include/flang/Parser/dump-parse-tree.h (-1)
(modified) flang/include/flang/Parser/parse-tree.h (+1-3)
(modified) flang/lib/Parser/openmp-parsers.cpp (+1-2)
(modified) flang/lib/Parser/unparse.cpp (+8-4)
(modified) flang/lib/Semantics/resolve-names.cpp (+22-5)
(modified) flang/test/Parser/OpenMP/declare-reduction-unparse.f90 (+2-2)
(modified) flang/test/Parser/OpenMP/metadirective-dirspec.f90 (+17-12)
(modified) flang/test/Semantics/OpenMP/declare-reduction.f90 (+7-2)

diff --git a/flang/include/flang/Parser/dump-parse-tree.h b/flang/include/flang/Parser/dump-parse-tree.h
index 3b3c6bdc448d7..72c6ecf9a6cf5 100644
--- a/flang/include/flang/Parser/dump-parse-tree.h
+++ b/flang/include/flang/Parser/dump-parse-tree.h
@@ -639,7 +639,6 @@ class ParseTreeDumper {
   NODE(parser, OmpTaskReductionClause)
   NODE(OmpTaskReductionClause, Modifier)
   NODE(parser, OmpInitializerProc)
-  NODE(parser, OmpInitializerExpr)
   NODE(parser, OmpInitializerClause)
   NODE(parser, OmpReductionIdentifier)
   NODE(parser, OmpAllocateClause)
diff --git a/flang/include/flang/Parser/parse-tree.h b/flang/include/flang/Parser/parse-tree.h
index f11859bb09ddb..7727746535441 100644
--- a/flang/include/flang/Parser/parse-tree.h
+++ b/flang/include/flang/Parser/parse-tree.h
@@ -4252,12 +4252,10 @@ struct OmpInitializerProc {
   TUPLE_CLASS_BOILERPLATE(OmpInitializerProc);
   std::tuple<ProcedureDesignator, std::list<ActualArgSpec>> t;
 };
-WRAPPER_CLASS(OmpInitializerExpr, Expr);
-
 // Initialization for declare reduction construct
 struct OmpInitializerClause {
   UNION_CLASS_BOILERPLATE(OmpInitializerClause);
-  std::variant<OmpInitializerProc, OmpInitializerExpr> u;
+  std::variant<OmpInitializerProc, AssignmentStmt> u;
 };
 
 // Ref: [4.5:199-201], [5.0:288-290], [5.1:321-322], [5.2:115-117]
diff --git a/flang/lib/Parser/openmp-parsers.cpp b/flang/lib/Parser/openmp-parsers.cpp
index 43cd2ea1eb0e6..bbce8b1c7b392 100644
--- a/flang/lib/Parser/openmp-parsers.cpp
+++ b/flang/lib/Parser/openmp-parsers.cpp
@@ -1176,12 +1176,11 @@ TYPE_PARSER(construct<OmpBlockDirective>(first(
 TYPE_PARSER(sourced(construct<OmpBeginBlockDirective>(
     sourced(Parser<OmpBlockDirective>{}), Parser<OmpClauseList>{})))
 
-TYPE_PARSER(construct<OmpInitializerExpr>("OMP_PRIV =" >> expr))
 TYPE_PARSER(construct<OmpInitializerProc>(Parser<ProcedureDesignator>{},
     parenthesized(many(maybe(","_tok) >> Parser<ActualArgSpec>{}))))
 
 TYPE_PARSER(construct<OmpInitializerClause>(
-    construct<OmpInitializerClause>(Parser<OmpInitializerExpr>{}) ||
+    construct<OmpInitializerClause>(assignmentStmt) ||
     construct<OmpInitializerClause>(Parser<OmpInitializerProc>{})))
 
 // 2.16 Declare Reduction Construct
diff --git a/flang/lib/Parser/unparse.cpp b/flang/lib/Parser/unparse.cpp
index 1df17b6d7382b..667da7533d3a8 100644
--- a/flang/lib/Parser/unparse.cpp
+++ b/flang/lib/Parser/unparse.cpp
@@ -2705,11 +2705,15 @@ class UnparseVisitor {
     Walk(std::get<std::list<ActualArgSpec>>(x.t));
     Put(")");
   }
-  void Unparse(const OmpInitializerExpr &x) {
-    Word("OMP_PRIV = ");
-    Walk(x.v);
+  void Unparse(const OmpInitializerClause &x) {
+    // Don't let the visitor go to the normal AssignmentStmt Unparse function,
+    // it adds an extra newline that we don't want.
+    if (const auto *assignment{std::get_if<AssignmentStmt>(&x.u)}) {
+      Walk(assignment->t, "=");
+    } else {
+      Walk(x.u);
+    }
   }
-  void Unparse(const OmpInitializerClause &x) { Walk(x.u); }
   void Unparse(const OpenMPDeclareReductionConstruct &x) {
     BeginOpenMP();
     Word("!$OMP DECLARE REDUCTION ");
diff --git a/flang/lib/Semantics/resolve-names.cpp b/flang/lib/Semantics/resolve-names.cpp
index 4f80cdca0f4bb..3e182c9289947 100644
--- a/flang/lib/Semantics/resolve-names.cpp
+++ b/flang/lib/Semantics/resolve-names.cpp
@@ -1761,11 +1761,28 @@ void OmpVisitor::ProcessReductionSpecifier(
   // Creating a new scope in case the combiner expression (or clauses) use
   // reerved identifiers, like "omp_in". This is a temporary solution until
   // we deal with these in a more thorough way.
-  PushScope(Scope::Kind::OtherConstruct, nullptr);
-  Walk(std::get<parser::OmpTypeNameList>(spec.t));
-  Walk(std::get<std::optional<parser::OmpReductionCombiner>>(spec.t));
-  Walk(clauses);
-  PopScope();
+  auto &typeList{std::get<parser::OmpTypeNameList>(spec.t)};
+  for (auto &t : typeList.v) {
+    PushScope(Scope::Kind::OtherConstruct, nullptr);
+    BeginDeclTypeSpec();
+    // We need to walk t.u because Walk(t) does it's own BeginDeclTypeSpec.
+    Walk(t.u);
+
+    const DeclTypeSpec *typeSpec{GetDeclTypeSpec()};
+    assert(typeSpec && "We should have a type here");
+    const parser::CharBlock ompVarNames[]{
+        {"omp_in", 6}, {"omp_out", 7}, {"omp_priv", 8}};
+
+    for (auto &nm : ompVarNames) {
+      ObjectEntityDetails details{};
+      details.set_type(*typeSpec);
+      MakeSymbol(nm, Attrs{}, std::move(details));
+    }
+    EndDeclTypeSpec();
+    Walk(std::get<std::optional<parser::OmpReductionCombiner>>(spec.t));
+    Walk(clauses);
+    PopScope();
+  }
 }
 
 bool OmpVisitor::Pre(const parser::OmpDirectiveSpecification &x) {
diff --git a/flang/test/Parser/OpenMP/declare-reduction-unparse.f90 b/flang/test/Parser/OpenMP/declare-reduction-unparse.f90
index f0ab99f5cfb5f..7b1b569d87f78 100644
--- a/flang/test/Parser/OpenMP/declare-reduction-unparse.f90
+++ b/flang/test/Parser/OpenMP/declare-reduction-unparse.f90
@@ -44,7 +44,7 @@ end function func
 program main
   integer :: my_var
 !CHECK: !$OMP DECLARE REDUCTION (my_add_red:INTEGER: omp_out=omp_out+omp_in
-!CHECK-NEXT: ) INITIALIZER(OMP_PRIV = 0_4)
+!CHECK-NEXT: ) INITIALIZER(omp_priv=0_4)
 
   !$omp declare reduction (my_add_red : integer : omp_out = omp_out + omp_in) initializer (omp_priv=0)
   my_var = 0
@@ -58,4 +58,4 @@ end program main
 !PARSE-TREE:        OmpReductionIdentifier -> ProcedureDesignator -> Name = 'my_add_red'
 !PARSE-TREE:        DeclarationTypeSpec -> IntrinsicTypeSpec -> IntegerTypeSpec
 !PARSE-TREE:        OmpReductionCombiner -> AssignmentStmt = 'omp_out=omp_out+omp_in'
-!PARSE-TREE:        OmpInitializerClause -> OmpInitializerExpr -> Expr = '0_4'
+!PARSE-TREE:        OmpInitializerClause -> AssignmentStmt = 'omp_priv=0_4'
diff --git a/flang/test/Parser/OpenMP/metadirective-dirspec.f90 b/flang/test/Parser/OpenMP/metadirective-dirspec.f90
index 73520c41fe77d..872cf5d46d34a 100644
--- a/flang/test/Parser/OpenMP/metadirective-dirspec.f90
+++ b/flang/test/Parser/OpenMP/metadirective-dirspec.f90
@@ -92,10 +92,10 @@ subroutine f03
     integer :: x
   endtype
   type :: tt2
-    real :: a
+    real :: x
   endtype
   !$omp metadirective when(user={condition(.true.)}: &
-  !$omp & declare reduction(+ : tt1, tt2 : omp_out = omp_in + omp_out))
+  !$omp & declare reduction(+ : tt1, tt2 : omp_out%x = omp_in%x + omp_out%x))
 end
 
 !UNPARSE: SUBROUTINE f03
@@ -103,9 +103,9 @@ subroutine f03
 !UNPARSE:   INTEGER :: x
 !UNPARSE:  END TYPE
 !UNPARSE:  TYPE :: tt2
-!UNPARSE:   REAL :: a
+!UNPARSE:   REAL :: x
 !UNPARSE:  END TYPE
-!UNPARSE: !$OMP METADIRECTIVE  WHEN(USER={CONDITION(.true._4)}: DECLARE REDUCTION(+:tt1,tt2: omp_out=omp_in+omp_out
+!UNPARSE: !$OMP METADIRECTIVE  WHEN(USER={CONDITION(.true._4)}: DECLARE REDUCTION(+:tt1,tt2: omp_out%x=omp_in%x+omp_out%x
 !UNPARSE: ))
 !UNPARSE: END SUBROUTINE
 
@@ -127,15 +127,20 @@ subroutine f03
 !PARSE-TREE: | | | | | Name = 'tt1'
 !PARSE-TREE: | | | | OmpTypeSpecifier -> TypeSpec -> DerivedTypeSpec
 !PARSE-TREE: | | | | | Name = 'tt2'
-!PARSE-TREE: | | | | OmpReductionCombiner -> AssignmentStmt = 'omp_out=omp_in+omp_out'
-!PARSE-TREE: | | | | | Variable = 'omp_out'
-!PARSE-TREE: | | | | | | Designator -> DataRef -> Name = 'omp_out'
-!PARSE-TREE: | | | | | Expr = 'omp_in+omp_out'
+!PARSE-TREE: | | | | OmpReductionCombiner -> AssignmentStmt = 'omp_out%x=omp_in%x+omp_out%x'
+!PARSE-TREE: | | | | | | Designator -> DataRef -> StructureComponent
+!PARSE-TREE: | | | | | | | DataRef -> Name = 'omp_out'
+!PARSE-TREE: | | | | | | | Name = 'x'
+!PARSE-TREE: | | | | | Expr = 'omp_in%x+omp_out%x'
 !PARSE-TREE: | | | | | | Add
-!PARSE-TREE: | | | | | | | Expr = 'omp_in'
-!PARSE-TREE: | | | | | | | | Designator -> DataRef -> Name = 'omp_in'
-!PARSE-TREE: | | | | | | | Expr = 'omp_out'
-!PARSE-TREE: | | | | | | | | Designator -> DataRef -> Name = 'omp_out'
+!PARSE-TREE: | | | | | | | Expr = 'omp_in%x'
+!PARSE-TREE: | | | | | | | | Designator -> DataRef -> StructureComponent
+!PARSE-TREE: | | | | | | | | | DataRef -> Name = 'omp_in'
+!PARSE-TREE: | | | | | | | | | Name = 'x'
+!PARSE-TREE: | | | | | | | Expr = 'omp_out%x'
+!PARSE-TREE: | | | | | | | | Designator -> DataRef -> StructureComponent
+!PARSE-TREE: | | | | | | | | | DataRef -> Name = 'omp_out'
+!PARSE-TREE: | | | | | | | | | Name = 'x'
 !PARSE-TREE: | | | OmpClauseList ->
 
 subroutine f04
diff --git a/flang/test/Semantics/OpenMP/declare-reduction.f90 b/flang/test/Semantics/OpenMP/declare-reduction.f90
index e61af0430575f..5bb2132b8eaa2 100644
--- a/flang/test/Semantics/OpenMP/declare-reduction.f90
+++ b/flang/test/Semantics/OpenMP/declare-reduction.f90
@@ -18,7 +18,10 @@ end subroutine initme
   end interface
   !$omp declare reduction(red_add:integer(4):omp_out=omp_out+omp_in) initializer(initme(omp_priv,0))
 !CHECK: red_add: Misc ConstructName
-!CHECK: Subprogram scope: initme  
+!CHECK: Subprogram scope: initme
+!CHECK: omp_in size=4 offset=0: ObjectEntity type: INTEGER(4)
+!CHECK: omp_out size=4 offset=4: ObjectEntity type: INTEGER(4)
+!CHECK: omp_priv size=4 offset=8: ObjectEntity type: INTEGER(4)
 !$omp simd reduction(red_add:res)
   do i=1,n
      res=res+x(i)
@@ -32,6 +35,8 @@ program main
   !$omp declare reduction (my_add_red : integer : omp_out = omp_out + omp_in) initializer (omp_priv=0)
 
 !CHECK: my_add_red: Misc ConstructName
+!CHECK: omp_in size=4 offset=0: ObjectEntity type: INTEGER(4)
+!CHECK: omp_out size=4 offset=4: ObjectEntity type: INTEGER(4)
+!CHECK: omp_priv size=4 offset=8: ObjectEntity type: INTEGER(4)
   
 end program main
-

llvmbot · 2025-03-06T12:23:03Z

@llvm/pr-subscribers-flang-parser

Author: Mats Petersson (Leporacanthicus)

Changes

…UCTION

This patch allows better parsing of the reduction and initializer components, including supporting derived types in both those places.

There is more work needed here, but this is a definite improvement in what can be handled through parser and semantics.

Note that declare reduction is still not supported in lowering, so any attempt to compile DECLARE REDUCTION code will end with a TODO aka "Not yet implemented" abort in the compiler.

Note that this version of the code does not cover declaring multiple reductions using the same name with different types. This is will be fixed in a future patch. [This was also the case before this change].

One existing test modified to actually compile (as it didn't in the original form).

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

8 Files Affected:

(modified) flang/include/flang/Parser/dump-parse-tree.h (-1)
(modified) flang/include/flang/Parser/parse-tree.h (+1-3)
(modified) flang/lib/Parser/openmp-parsers.cpp (+1-2)
(modified) flang/lib/Parser/unparse.cpp (+8-4)
(modified) flang/lib/Semantics/resolve-names.cpp (+22-5)
(modified) flang/test/Parser/OpenMP/declare-reduction-unparse.f90 (+2-2)
(modified) flang/test/Parser/OpenMP/metadirective-dirspec.f90 (+17-12)
(modified) flang/test/Semantics/OpenMP/declare-reduction.f90 (+7-2)

diff --git a/flang/include/flang/Parser/dump-parse-tree.h b/flang/include/flang/Parser/dump-parse-tree.h
index 3b3c6bdc448d7..72c6ecf9a6cf5 100644
--- a/flang/include/flang/Parser/dump-parse-tree.h
+++ b/flang/include/flang/Parser/dump-parse-tree.h
@@ -639,7 +639,6 @@ class ParseTreeDumper {
   NODE(parser, OmpTaskReductionClause)
   NODE(OmpTaskReductionClause, Modifier)
   NODE(parser, OmpInitializerProc)
-  NODE(parser, OmpInitializerExpr)
   NODE(parser, OmpInitializerClause)
   NODE(parser, OmpReductionIdentifier)
   NODE(parser, OmpAllocateClause)
diff --git a/flang/include/flang/Parser/parse-tree.h b/flang/include/flang/Parser/parse-tree.h
index f11859bb09ddb..7727746535441 100644
--- a/flang/include/flang/Parser/parse-tree.h
+++ b/flang/include/flang/Parser/parse-tree.h
@@ -4252,12 +4252,10 @@ struct OmpInitializerProc {
   TUPLE_CLASS_BOILERPLATE(OmpInitializerProc);
   std::tuple<ProcedureDesignator, std::list<ActualArgSpec>> t;
 };
-WRAPPER_CLASS(OmpInitializerExpr, Expr);
-
 // Initialization for declare reduction construct
 struct OmpInitializerClause {
   UNION_CLASS_BOILERPLATE(OmpInitializerClause);
-  std::variant<OmpInitializerProc, OmpInitializerExpr> u;
+  std::variant<OmpInitializerProc, AssignmentStmt> u;
 };
 
 // Ref: [4.5:199-201], [5.0:288-290], [5.1:321-322], [5.2:115-117]
diff --git a/flang/lib/Parser/openmp-parsers.cpp b/flang/lib/Parser/openmp-parsers.cpp
index 43cd2ea1eb0e6..bbce8b1c7b392 100644
--- a/flang/lib/Parser/openmp-parsers.cpp
+++ b/flang/lib/Parser/openmp-parsers.cpp
@@ -1176,12 +1176,11 @@ TYPE_PARSER(construct<OmpBlockDirective>(first(
 TYPE_PARSER(sourced(construct<OmpBeginBlockDirective>(
     sourced(Parser<OmpBlockDirective>{}), Parser<OmpClauseList>{})))
 
-TYPE_PARSER(construct<OmpInitializerExpr>("OMP_PRIV =" >> expr))
 TYPE_PARSER(construct<OmpInitializerProc>(Parser<ProcedureDesignator>{},
     parenthesized(many(maybe(","_tok) >> Parser<ActualArgSpec>{}))))
 
 TYPE_PARSER(construct<OmpInitializerClause>(
-    construct<OmpInitializerClause>(Parser<OmpInitializerExpr>{}) ||
+    construct<OmpInitializerClause>(assignmentStmt) ||
     construct<OmpInitializerClause>(Parser<OmpInitializerProc>{})))
 
 // 2.16 Declare Reduction Construct
diff --git a/flang/lib/Parser/unparse.cpp b/flang/lib/Parser/unparse.cpp
index 1df17b6d7382b..667da7533d3a8 100644
--- a/flang/lib/Parser/unparse.cpp
+++ b/flang/lib/Parser/unparse.cpp
@@ -2705,11 +2705,15 @@ class UnparseVisitor {
     Walk(std::get<std::list<ActualArgSpec>>(x.t));
     Put(")");
   }
-  void Unparse(const OmpInitializerExpr &x) {
-    Word("OMP_PRIV = ");
-    Walk(x.v);
+  void Unparse(const OmpInitializerClause &x) {
+    // Don't let the visitor go to the normal AssignmentStmt Unparse function,
+    // it adds an extra newline that we don't want.
+    if (const auto *assignment{std::get_if<AssignmentStmt>(&x.u)}) {
+      Walk(assignment->t, "=");
+    } else {
+      Walk(x.u);
+    }
   }
-  void Unparse(const OmpInitializerClause &x) { Walk(x.u); }
   void Unparse(const OpenMPDeclareReductionConstruct &x) {
     BeginOpenMP();
     Word("!$OMP DECLARE REDUCTION ");
diff --git a/flang/lib/Semantics/resolve-names.cpp b/flang/lib/Semantics/resolve-names.cpp
index 4f80cdca0f4bb..3e182c9289947 100644
--- a/flang/lib/Semantics/resolve-names.cpp
+++ b/flang/lib/Semantics/resolve-names.cpp
@@ -1761,11 +1761,28 @@ void OmpVisitor::ProcessReductionSpecifier(
   // Creating a new scope in case the combiner expression (or clauses) use
   // reerved identifiers, like "omp_in". This is a temporary solution until
   // we deal with these in a more thorough way.
-  PushScope(Scope::Kind::OtherConstruct, nullptr);
-  Walk(std::get<parser::OmpTypeNameList>(spec.t));
-  Walk(std::get<std::optional<parser::OmpReductionCombiner>>(spec.t));
-  Walk(clauses);
-  PopScope();
+  auto &typeList{std::get<parser::OmpTypeNameList>(spec.t)};
+  for (auto &t : typeList.v) {
+    PushScope(Scope::Kind::OtherConstruct, nullptr);
+    BeginDeclTypeSpec();
+    // We need to walk t.u because Walk(t) does it's own BeginDeclTypeSpec.
+    Walk(t.u);
+
+    const DeclTypeSpec *typeSpec{GetDeclTypeSpec()};
+    assert(typeSpec && "We should have a type here");
+    const parser::CharBlock ompVarNames[]{
+        {"omp_in", 6}, {"omp_out", 7}, {"omp_priv", 8}};
+
+    for (auto &nm : ompVarNames) {
+      ObjectEntityDetails details{};
+      details.set_type(*typeSpec);
+      MakeSymbol(nm, Attrs{}, std::move(details));
+    }
+    EndDeclTypeSpec();
+    Walk(std::get<std::optional<parser::OmpReductionCombiner>>(spec.t));
+    Walk(clauses);
+    PopScope();
+  }
 }
 
 bool OmpVisitor::Pre(const parser::OmpDirectiveSpecification &x) {
diff --git a/flang/test/Parser/OpenMP/declare-reduction-unparse.f90 b/flang/test/Parser/OpenMP/declare-reduction-unparse.f90
index f0ab99f5cfb5f..7b1b569d87f78 100644
--- a/flang/test/Parser/OpenMP/declare-reduction-unparse.f90
+++ b/flang/test/Parser/OpenMP/declare-reduction-unparse.f90
@@ -44,7 +44,7 @@ end function func
 program main
   integer :: my_var
 !CHECK: !$OMP DECLARE REDUCTION (my_add_red:INTEGER: omp_out=omp_out+omp_in
-!CHECK-NEXT: ) INITIALIZER(OMP_PRIV = 0_4)
+!CHECK-NEXT: ) INITIALIZER(omp_priv=0_4)
 
   !$omp declare reduction (my_add_red : integer : omp_out = omp_out + omp_in) initializer (omp_priv=0)
   my_var = 0
@@ -58,4 +58,4 @@ end program main
 !PARSE-TREE:        OmpReductionIdentifier -> ProcedureDesignator -> Name = 'my_add_red'
 !PARSE-TREE:        DeclarationTypeSpec -> IntrinsicTypeSpec -> IntegerTypeSpec
 !PARSE-TREE:        OmpReductionCombiner -> AssignmentStmt = 'omp_out=omp_out+omp_in'
-!PARSE-TREE:        OmpInitializerClause -> OmpInitializerExpr -> Expr = '0_4'
+!PARSE-TREE:        OmpInitializerClause -> AssignmentStmt = 'omp_priv=0_4'
diff --git a/flang/test/Parser/OpenMP/metadirective-dirspec.f90 b/flang/test/Parser/OpenMP/metadirective-dirspec.f90
index 73520c41fe77d..872cf5d46d34a 100644
--- a/flang/test/Parser/OpenMP/metadirective-dirspec.f90
+++ b/flang/test/Parser/OpenMP/metadirective-dirspec.f90
@@ -92,10 +92,10 @@ subroutine f03
     integer :: x
   endtype
   type :: tt2
-    real :: a
+    real :: x
   endtype
   !$omp metadirective when(user={condition(.true.)}: &
-  !$omp & declare reduction(+ : tt1, tt2 : omp_out = omp_in + omp_out))
+  !$omp & declare reduction(+ : tt1, tt2 : omp_out%x = omp_in%x + omp_out%x))
 end
 
 !UNPARSE: SUBROUTINE f03
@@ -103,9 +103,9 @@ subroutine f03
 !UNPARSE:   INTEGER :: x
 !UNPARSE:  END TYPE
 !UNPARSE:  TYPE :: tt2
-!UNPARSE:   REAL :: a
+!UNPARSE:   REAL :: x
 !UNPARSE:  END TYPE
-!UNPARSE: !$OMP METADIRECTIVE  WHEN(USER={CONDITION(.true._4)}: DECLARE REDUCTION(+:tt1,tt2: omp_out=omp_in+omp_out
+!UNPARSE: !$OMP METADIRECTIVE  WHEN(USER={CONDITION(.true._4)}: DECLARE REDUCTION(+:tt1,tt2: omp_out%x=omp_in%x+omp_out%x
 !UNPARSE: ))
 !UNPARSE: END SUBROUTINE
 
@@ -127,15 +127,20 @@ subroutine f03
 !PARSE-TREE: | | | | | Name = 'tt1'
 !PARSE-TREE: | | | | OmpTypeSpecifier -> TypeSpec -> DerivedTypeSpec
 !PARSE-TREE: | | | | | Name = 'tt2'
-!PARSE-TREE: | | | | OmpReductionCombiner -> AssignmentStmt = 'omp_out=omp_in+omp_out'
-!PARSE-TREE: | | | | | Variable = 'omp_out'
-!PARSE-TREE: | | | | | | Designator -> DataRef -> Name = 'omp_out'
-!PARSE-TREE: | | | | | Expr = 'omp_in+omp_out'
+!PARSE-TREE: | | | | OmpReductionCombiner -> AssignmentStmt = 'omp_out%x=omp_in%x+omp_out%x'
+!PARSE-TREE: | | | | | | Designator -> DataRef -> StructureComponent
+!PARSE-TREE: | | | | | | | DataRef -> Name = 'omp_out'
+!PARSE-TREE: | | | | | | | Name = 'x'
+!PARSE-TREE: | | | | | Expr = 'omp_in%x+omp_out%x'
 !PARSE-TREE: | | | | | | Add
-!PARSE-TREE: | | | | | | | Expr = 'omp_in'
-!PARSE-TREE: | | | | | | | | Designator -> DataRef -> Name = 'omp_in'
-!PARSE-TREE: | | | | | | | Expr = 'omp_out'
-!PARSE-TREE: | | | | | | | | Designator -> DataRef -> Name = 'omp_out'
+!PARSE-TREE: | | | | | | | Expr = 'omp_in%x'
+!PARSE-TREE: | | | | | | | | Designator -> DataRef -> StructureComponent
+!PARSE-TREE: | | | | | | | | | DataRef -> Name = 'omp_in'
+!PARSE-TREE: | | | | | | | | | Name = 'x'
+!PARSE-TREE: | | | | | | | Expr = 'omp_out%x'
+!PARSE-TREE: | | | | | | | | Designator -> DataRef -> StructureComponent
+!PARSE-TREE: | | | | | | | | | DataRef -> Name = 'omp_out'
+!PARSE-TREE: | | | | | | | | | Name = 'x'
 !PARSE-TREE: | | | OmpClauseList ->
 
 subroutine f04
diff --git a/flang/test/Semantics/OpenMP/declare-reduction.f90 b/flang/test/Semantics/OpenMP/declare-reduction.f90
index e61af0430575f..5bb2132b8eaa2 100644
--- a/flang/test/Semantics/OpenMP/declare-reduction.f90
+++ b/flang/test/Semantics/OpenMP/declare-reduction.f90
@@ -18,7 +18,10 @@ end subroutine initme
   end interface
   !$omp declare reduction(red_add:integer(4):omp_out=omp_out+omp_in) initializer(initme(omp_priv,0))
 !CHECK: red_add: Misc ConstructName
-!CHECK: Subprogram scope: initme  
+!CHECK: Subprogram scope: initme
+!CHECK: omp_in size=4 offset=0: ObjectEntity type: INTEGER(4)
+!CHECK: omp_out size=4 offset=4: ObjectEntity type: INTEGER(4)
+!CHECK: omp_priv size=4 offset=8: ObjectEntity type: INTEGER(4)
 !$omp simd reduction(red_add:res)
   do i=1,n
      res=res+x(i)
@@ -32,6 +35,8 @@ program main
   !$omp declare reduction (my_add_red : integer : omp_out = omp_out + omp_in) initializer (omp_priv=0)
 
 !CHECK: my_add_red: Misc ConstructName
+!CHECK: omp_in size=4 offset=0: ObjectEntity type: INTEGER(4)
+!CHECK: omp_out size=4 offset=4: ObjectEntity type: INTEGER(4)
+!CHECK: omp_priv size=4 offset=8: ObjectEntity type: INTEGER(4)
   
 end program main
-

kiranchandramohan · 2025-03-07T22:25:23Z

flang/lib/Semantics/resolve-names.cpp

+    const parser::CharBlock ompVarNames[]{
+        {"omp_in", 6}, {"omp_out", 7}, {"omp_priv", 8}};


Nit: This can be outside the loop.

kiranchandramohan · 2025-03-07T22:34:15Z

flang/lib/Semantics/resolve-names.cpp

+    const parser::CharBlock ompVarNames[] = {
+        {"omp_in", 6}, {"omp_out", 7}, {"omp_priv", 8}};


Are you taking advantage of the fact that the only variables that are permitted in the Combiner and Initializer are omp_in, omp_out and omp_priv? Hence there is no need to traverse to these names because we already know them?

The points I am not following are:

If we create symbols like these do these symbols get attached to the parser::Name in the Assignment Statement?

What happens when there are multiple types in the typelist? Since only the type has changed, do the ones created in the latter trips of the loop override the ones created previously.

omp_orig can also appear in the initializer.

Since there is a loop here, we should have at least a test with more than one type.

github-actions · 2025-03-11T15:53:44Z

✅ With the latest revision this PR passed the C/C++ code formatter.

kiranchandramohan

LG. Thanks @Leporacanthicus.

llvm#129908) …UCTION This patch allows better parsing of the reduction and initializer components, including supporting derived types in both those places. There is more work needed here, but this is a definite improvement in what can be handled through parser and semantics. Note that declare reduction is still not supported in lowering, so any attempt to compile DECLARE REDUCTION code will end with a TODO aka "Not yet implemented" abort in the compiler. Note that this version of the code does not cover declaring multiple reductions using the same name with different types. This is will be fixed in a future patch. [This was also the case before this change]. One existing test modified to actually compile (as it didn't in the original form).

Leporacanthicus requested a review from kparzysz March 5, 2025 18:20

kparzysz approved these changes Mar 5, 2025

View reviewed changes

kiranchandramohan reviewed Mar 6, 2025

View reviewed changes

Leporacanthicus added 2 commits March 6, 2025 11:31

Review comment fixes

df74037

Move function inline

2708306

Leporacanthicus marked this pull request as ready for review March 6, 2025 12:22

llvmbot added flang Flang issues not falling into any other category flang:openmp flang:semantics flang:parser labels Mar 6, 2025

kiranchandramohan reviewed Mar 7, 2025

View reviewed changes

Leporacanthicus added 2 commits March 10, 2025 12:42

Add comments

05c5f83

Add omp_orig

1c8e0b9

Fix formatting

f83bcd8

kiranchandramohan approved these changes Mar 12, 2025

View reviewed changes

Leporacanthicus merged commit d0188eb into llvm:main Mar 13, 2025
11 checks passed

		const parser::CharBlock ompVarNames[] = {
		{"omp_in", 6}, {"omp_out", 7}, {"omp_priv", 8}};

		const parser::CharBlock ompVarNames[]{
		{"omp_in", 6}, {"omp_out", 7}, {"omp_priv", 8}};

[flang][OpenMP]Add symbls omp_in, omp_out and omp_priv in DECLARE RED… #129908

[flang][OpenMP]Add symbls omp_in, omp_out and omp_priv in DECLARE RED… #129908

Uh oh!

Conversation

Leporacanthicus commented Mar 5, 2025

Uh oh!

kparzysz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

llvmbot commented Mar 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Mar 6, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Mar 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kiranchandramohan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

llvmbot commented Mar 6, 2025 •

edited

Loading

github-actions bot commented Mar 11, 2025 •

edited

Loading