[flang][openmp]Add UserReductionDetails and use in DECLARE REDUCTION #140066

tblah · 2025-05-15T14:28:18Z

This adds another puzzle piece for the support of OpenMP DECLARE REDUCTION functionality.

This adds support for operators with derived types, as well as declaring multiple different types with the same name or operator.

A new detail class for UserReductionDetials is introduced to hold the list of types supported for a given reduction declaration.

Tests for parsing and symbol generation added.

Declare reduction is still not supported to lowering, it will generate a "Not yet implemented" fatal error.

tblah · 2025-05-15T14:29:54Z

This is a continuation of @Leporacanthicus's work in #131628.

tblah · 2025-05-21T09:05:53Z

ping for review

flang/lib/Semantics/check-omp-structure.cpp

kiranchandramohan · 2025-05-30T09:39:58Z

flang/include/flang/Semantics/semantics.h

+  // Store (and get a reference to the stored string) for mangled names
+  // used for OpenMP DECLARE REDUCTION.
+  std::string &StoreUserReductionName(const std::string &name);


I was hoping we could get this to work without the requirement for a string storage like this.

SaveTempName is used elsewhere for saving mangled names so I'm not adding any new confusion here. There's currently no code removing any strings that are added via that helper so the lifetime should be okay.

kiranchandramohan · 2025-05-30T12:27:06Z

flang/lib/Semantics/resolve-names.cpp

 void OmpVisitor::ProcessReductionSpecifier(
    const parser::OmpReductionSpecifier &spec,
-    const std::optional<parser::OmpClauseList> &clauses) {
+    const std::optional<parser::OmpClauseList> &clauses,


The changes in this function looks a bit hacky.
Ideally we shouldn't have random parser::Name not attached to the parse-tree and floating symbols.

github-actions · 2025-06-04T14:57:48Z

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

NimishMishra

LGTM overall. But please wait for another approval

NimishMishra · 2025-06-04T15:06:57Z

flang/include/flang/Semantics/symbol.h

+
+  UserReductionDetails() = default;
+
+  void AddType(const DeclTypeSpec &type) { typeList_.push_back(&type); }


Nit (feel free to ignore): emplace_back may be preferable for non-trivial types in std::vector

I ignored here (because it is just adding a pointer). But the other one does benefit from emplace_back to construct the variant in place. Thanks

NimishMishra · 2025-06-04T15:31:37Z

flang/lib/Semantics/check-omp-structure.cpp

+      return CheckSymbolSupportsType(
+          scope, MangleDefinedOperator(definedOp->v.symbol->name()), type);
+    }
+    DIE("Intrinsic Operator not found - parsing gone wrong?");


Nit: Should this be DIE or a semantic error that sources the offending source?

I'll replace this with unreachable to make it clearer

kiranchandramohan · 2025-06-04T16:40:55Z

Could you check the format complaints again?

This adds another puzzle piece for the support of OpenMP DECLARE REDUCTION functionality. This adds support for operators with derived types, as well as declaring multiple different types with the same name or operator. A new detail class for UserReductionDetials is introduced to hold the list of types supported for a given reduction declaration. Tests for parsing and symbol generation added. Declare reduction is still not supported to lowering, it will generate a "Not yet implemented" fatal error.

* Add two more tests (multiple operator-based declarations and re-using symbol already declared. * Add a few comments. * Fix up logical results.

Also print the reduction declaration in the module file. Fix trivial typo. Add/modify tests to cover all the new things, including fixing the duplicated typo in the test...

Also rebase, as the branch was quite a way behind. Small conflict was resolved.

Add code to better handle operators in parsing and semantics. Add a function to set the the scope when processign assignments, which caused a crash in "check for pure functions". Add three new tests and amend existing tests to cover a pure function.

I had to rebase to reproduce the test failure

kiranchandramohan

We will probably have to revisit when we implement the lowering.

Please check whether the reported issues error out properly now.
#141306
#119960
#97241
#92832
#66453
#57219

LG.

kiranchandramohan · 2025-06-06T16:01:11Z

flang/lib/Semantics/mod-file.cpp

+  const auto &details{symbol.get<UserReductionDetails>()};
+  // The module content for a OpenMP Declare Reduction is the OpenMP
+  // declaration. There may be multiple declarations.
+  // Decls are pointers, so do not use a referene.


Suggested change

// Decls are pointers, so do not use a referene.

// Decls are pointers, so do not use a reference.

kiranchandramohan · 2025-06-06T16:01:40Z

flang/lib/Semantics/resolve-names.cpp

+    name = std::get_if<parser::Name>(&procDes->u);
+    // This shouldn't be a procedure component: this is the name of the
+    // reduction being declared.
+    assert(name && "ProcedureDesignator contained ProcComponentRef");


Could you use the CHECK macro?

tblah requested review from kiranchandramohan, NimishMishra and kparzysz May 15, 2025 14:28

tblah mentioned this pull request May 15, 2025

[flang][openmp]Add UserReductionDetails and use in DECLARE REDUCTION #131628

Closed

tblah force-pushed the ecclescake/omp-reduction-add-details branch from c4e01f6 to c5f9768 Compare May 19, 2025 17:11

kiranchandramohan reviewed May 30, 2025

View reviewed changes

tblah force-pushed the ecclescake/omp-reduction-add-details branch from bb650c5 to 9a7d1ab Compare June 4, 2025 14:54

NimishMishra approved these changes Jun 4, 2025

View reviewed changes

Leporacanthicus and others added 18 commits June 5, 2025 11:10

Fix review comments

4809f3e

* Add two more tests (multiple operator-based declarations and re-using symbol already declared. * Add a few comments. * Fix up logical results.

Use stringswitch and spell details correctly

dea0887

Add support for user defined operators in declare reduction

304575d

Also print the reduction declaration in the module file. Fix trivial typo. Add/modify tests to cover all the new things, including fixing the duplicated typo in the test...

Fix nit comments and add simple bad operator test

ebe6df8

Fix error messages to be more consistent

1cdc4e4

add missed test change

647d607

Improve support for metadirective + declare reduction

eebd884

Fix Klausler reported review comments

7422a16

Also rebase, as the branch was quite a way behind. Small conflict was resolved.

Fix some semantics issues

136b45b

Fix review comments

367dee6

Fix typo in test

e973f03

Use braced initialization

3e947ea

Don't create a new parser::Name on the stack

f38de57

Remove StoreUserReductionName

70462ee

Clang format

d147b15

Use emplace_back for non-trivial vector element type

672edfa

tblah added 3 commits June 5, 2025 11:10

Change error to unreachable

546add0

Manual clang-format

d364848

Fix tests

d12203c

I had to rebase to reproduce the test failure

tblah force-pushed the ecclescake/omp-reduction-add-details branch from ea6901d to d12203c Compare June 5, 2025 11:21

tblah requested a review from kiranchandramohan June 5, 2025 15:06

kiranchandramohan approved these changes Jun 6, 2025

View reviewed changes


		UserReductionDetails() = default;

		void AddType(const DeclTypeSpec &type) { typeList_.push_back(&type); }

	// Decls are pointers, so do not use a referene.
	// Decls are pointers, so do not use a reference.

[flang][openmp]Add UserReductionDetails and use in DECLARE REDUCTION #140066

Are you sure you want to change the base?

[flang][openmp]Add UserReductionDetails and use in DECLARE REDUCTION #140066

Conversation

tblah commented May 15, 2025

Uh oh!

tblah commented May 15, 2025

Uh oh!

tblah commented May 21, 2025

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!

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

Uh oh!

Uh oh!

NimishMishra 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kiranchandramohan commented Jun 4, 2025

Uh oh!

kiranchandramohan 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!

github-actions bot commented Jun 4, 2025 •

edited

Loading