Page MenuHomePhabricator

[flang][openmp] Make Reduction clause part of OmpClause
ClosedPublic

Authored by clementval on Dec 17 2020, 12:12 PM.

Details

Summary

After discussion in D93105 we found that the reduction clause was not following
the common OmpClause convention. This patch makes reduction clause part of OmpClause
with a value of OmpReductionClause in a similar way than task_reduction.
The unparse function for OmpReductionClause is adapted since the keyword and parenthesis
are issued by the corresponding unparse function for parser::OmpClause::Reduction.

Diff Detail

Event Timeline

clementval created this revision.Dec 17 2020, 12:12 PM
clementval requested review of this revision.Dec 17 2020, 12:12 PM
Herald added a project: Restricted Project. · View Herald Transcript
clementval added a project: Restricted Project.Dec 17 2020, 12:36 PM

+1 for finding the issue.
Thanks for the efforts and LGMT.

sameeranjoshi accepted this revision.Dec 18 2020, 8:19 AM
This revision is now accepted and ready to land.Dec 18 2020, 8:19 AM
sameeranjoshi added inline comments.Dec 18 2020, 8:41 AM
flang/lib/Semantics/check-omp-structure.cpp
498–499

A quick question on this-
Why are they not modeled as OmpClause nodes any reason ?
As I see all of them seem to have definition inside OMP.td file.

clementval added inline comments.Dec 18 2020, 8:57 AM
flang/lib/Semantics/check-omp-structure.cpp
498–499

Any clause will have a definition in OMP.td. If they have a flangClass defined it will remove them from OmpClause.

I guess we can have a look at all of them and see if there is still a good reason why they are not part of OmpClause. Might be a technical-debt from the refactoring when we switch to TableGen. In OpenACC there is no such clause.

Let me know if you want to have a look at this. Otherwise I can do it.

sameeranjoshi added inline comments.Dec 18 2020, 9:10 AM
flang/lib/Semantics/check-omp-structure.cpp
498–499

Let's start with OMPC_Allocate clause.
And the reference patch is D93482
What does technical-debt mean ?

clementval added inline comments.Dec 18 2020, 11:15 AM
flang/lib/Semantics/check-omp-structure.cpp
498–499

Just mean that the initial approach choose a simple path instead of a generic enough one. So when we refactored it we kept some of it and now it's good time to move everything to the same concept.

yhegde added inline comments.Dec 18 2020, 9:51 PM
flang/lib/Semantics/check-omp-structure.cpp
421

Can we get an Enter function (Enter(const parser::OmpClause::Reduction &x)) to check the restrictions on Reduction clause.

clementval marked an inline comment as done.Dec 19 2020, 4:29 AM
clementval added inline comments.
flang/lib/Semantics/check-omp-structure.cpp
421

Can be added when necessary

clementval marked an inline comment as done.

Rebase

This revision was landed with ongoing or failed builds.Jan 4 2021, 12:19 PM
This revision was automatically updated to reflect the committed changes.