Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Leporacanthicus merged 6 commits into from
Mar 13, 2025
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
eaff8c5
[flang][OpenMP]Add symbls omp_in, omp_out and omp_priv in DECLARE RED…
Leporacanthicus Feb 27, 2025
df74037
Review comment fixes
Leporacanthicus Mar 6, 2025
2708306
Move function inline
Leporacanthicus Mar 6, 2025
05c5f83
Add comments
Leporacanthicus Mar 10, 2025
1c8e0b9
Add omp_orig
Leporacanthicus Mar 11, 2025
f83bcd8
Fix formatting
Leporacanthicus Mar 12, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion flang/include/flang/Parser/dump-parse-tree.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
4 changes: 1 addition & 3 deletions flang/include/flang/Parser/parse-tree.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand Down
3 changes: 1 addition & 2 deletions flang/lib/Parser/openmp-parsers.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
11 changes: 7 additions & 4 deletions flang/lib/Parser/unparse.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2705,11 +2705,14 @@ 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);
Copy link
Contributor

@kparzysz kparzysz Mar 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@Leporacanthicus Leporacanthicus Mar 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

}
void Unparse(const OmpInitializerClause &x) { Walk(x.u); }
void Unparse(const OpenMPDeclareReductionConstruct &x) {
BeginOpenMP();
Word("!$OMP DECLARE REDUCTION ");
Expand Down
34 changes: 29 additions & 5 deletions flang/lib/Semantics/resolve-names.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1656,6 +1656,7 @@ class OmpVisitor : public virtual DeclarationVisitor {
const parser::OmpClauseList &clauses);
void ProcessReductionSpecifier(const parser::OmpReductionSpecifier &spec,
const std::optional<parser::OmpClauseList> &clauses);
void CreateTempSymbol(const parser::CharBlock &name, const DeclTypeSpec &dts);
};

bool OmpVisitor::NeedsScope(const parser::OpenMPBlockConstruct &x) {
Expand Down Expand Up @@ -1748,6 +1749,14 @@ void OmpVisitor::ProcessMapperSpecifier(const parser::OmpMapperSpecifier &spec,
PopScope();
}

void OmpVisitor::CreateTempSymbol(
const parser::CharBlock &name, const DeclTypeSpec &dts) {
ObjectEntityDetails details;
details.set_type(dts);

MakeSymbol(name, Attrs{}, std::move(details));
}

void OmpVisitor::ProcessReductionSpecifier(
const parser::OmpReductionSpecifier &spec,
const std::optional<parser::OmpClauseList> &clauses) {
Expand All @@ -1761,11 +1770,26 @@ 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);

auto *typeSpec = GetDeclTypeSpec();
assert(typeSpec && "We should have a type here");
const parser::CharBlock ompVarNames[] = {
{"omp_in", 6}, {"omp_out", 7}, {"omp_priv", 8}};
Copy link
Contributor

@kiranchandramohan kiranchandramohan Mar 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Braced init if possible.

Copy link
Contributor Author

@Leporacanthicus Leporacanthicus Mar 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@kiranchandramohan kiranchandramohan Mar 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) { ?

Copy link
Contributor Author

@Leporacanthicus Leporacanthicus Mar 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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].

Copy link
Contributor

@kiranchandramohan kiranchandramohan Mar 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@Leporacanthicus Leporacanthicus Mar 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor

@kiranchandramohan kiranchandramohan Mar 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Contributor

@kparzysz kparzysz Mar 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@Leporacanthicus Leporacanthicus Mar 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@Leporacanthicus Leporacanthicus Mar 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


for (auto &nm : ompVarNames) {
CreateTempSymbol(nm, *typeSpec);
}
EndDeclTypeSpec();
Walk(std::get<std::optional<parser::OmpReductionCombiner>>(spec.t));
Walk(clauses);
PopScope();
}
}

bool OmpVisitor::Pre(const parser::OmpDirectiveSpecification &x) {
Expand Down
4 changes: 2 additions & 2 deletions flang/test/Parser/OpenMP/declare-reduction-unparse.f90
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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'
29 changes: 17 additions & 12 deletions flang/test/Parser/OpenMP/metadirective-dirspec.f90
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,20 +92,20 @@ 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
!UNPARSE: TYPE :: tt1
!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

Expand All @@ -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
Expand Down