This is an archive of the discontinued LLVM Phabricator instance.

[Flang][openmp][5.0] Add task_reduction clause.
ClosedPublic

Authored by sameeranjoshi on Dec 11 2020, 4:36 AM.

Details

Summary

See OMP-5.0 2.19.5.5 task_reduction Clause.
To add a positive test case we need taskgroup directive which is not added hence skipping the test.
This is a dependency for taskgroup construct.

Co-authored-by: Valentin Clement <clementval@gmail.com>

Diff Detail

Event Timeline

sameeranjoshi created this revision.Dec 11 2020, 4:36 AM
sameeranjoshi requested review of this revision.Dec 11 2020, 4:37 AM
Herald added a project: Restricted Project. · View Herald Transcript
sameeranjoshi edited the summary of this revision. (Show Details)Dec 11 2020, 4:39 AM
sameeranjoshi added reviewers: clementval, kiranktp.
sameeranjoshi added a project: Restricted Project.
sameeranjoshi edited the summary of this revision. (Show Details)
sameeranjoshi removed subscribers: yaxunl, sstefan1, guansong.
clementval added inline comments.Dec 14 2020, 11:01 AM
flang/include/flang/Parser/parse-tree.h
3427

Is there a problem with the formatting here?

3430

Is there a good reason why OmpReductionClause takes a list of Designator and here you take an OmpObjectList?

flang/lib/Parser/unparse.cpp
2020

Extra blank line.

sameeranjoshi added inline comments.Dec 14 2020, 11:12 AM
flang/include/flang/Parser/parse-tree.h
3430

I did ask once the same in slack channel.
But I think the complicated checks can be done in semantic part given the tablegen framework.
https://flang-compiler.slack.com/archives/CGB8G85QV/p1603382608040500

clementval added inline comments.Dec 14 2020, 11:18 AM
flang/include/flang/Parser/parse-tree.h
3430

I think they should be the same and possibly use the same node for that.

sameeranjoshi marked 4 inline comments as done.

Update OmpReductionClause to use OmpObjectList.
Make OmpTaskReductionClause depend on OmpReductionClause.
This change was needed to create consistency among the reduction clauses.

Thank you for reviewing.

flang/include/flang/Parser/parse-tree.h
3430

OmpReductionClause now uses OmpObjectList.

clementval added inline comments.Dec 16 2020, 6:17 AM
flang/lib/Parser/openmp-parsers.cpp
108

Do you really need this indirection? Why not using OmpReductionClause directly?

Remove indirection and use OmpReductionClause directly.

clementval added inline comments.Dec 17 2020, 11:41 AM
flang/lib/Parser/openmp-parsers.cpp
226

@sameeranjoshi After our slack discussion, I hope you don't mind but I updated your patch with a working version without the indirection.

The reduction clause is actually not done "right" at the moment since it doesn't follow other clauses. It should be done in a similar way as TASK_REDUCTION now. I guess we can do this in a separated patch.

flang/lib/Parser/unparse.cpp
2007

This will be changed in a follow up patch to reflect the change in TaskReduction and Reduction.

clementval accepted this revision.Dec 17 2020, 11:51 AM

I guess it looks like what we have discussed now.

If you want to attribute correctly the co-authorship of this patch, you can add Co-authored-by: Valentin Clement <clementval@gmail.com> at the end of the commit message. GitHub will do the rest.

This revision is now accepted and ready to land.Dec 17 2020, 11:51 AM
kiranchandramohan added a subscriber: yhegde.

@yhegde Will the changes in this patch affect your reduction patch (https://reviews.llvm.org/D90697)?

clementval added inline comments.
flang/lib/Parser/openmp-parsers.cpp
226

update to reduction clause are done in D93482

@yhegde Will the changes in this patch affect your reduction patch (https://reviews.llvm.org/D90697)?

Will take a look and get back to you.

@yhegde Will the changes in this patch affect your reduction patch (https://reviews.llvm.org/D90697)?

Yes. Rt now I am directly getting the list of Designators . Now it will become a level of indirection with the list of OmpObjects.
I may be missing something but OpenMP 4.5 and 5.0 Reduction clause restrictions looks very similar , why this one level of indirection required?! Is it because functions names in Reduction clause can come from common blocks ?!

@yhegde Will the changes in this patch affect your reduction patch (https://reviews.llvm.org/D90697)?

Yes. Rt now I am directly getting the list of Designators . Now it will become a level of indirection with the list of OmpObjects.
I may be missing something but OpenMP 4.5 and 5.0 Reduction clause restrictions looks very similar , why this one level of indirection required?! Is it because functions names in Reduction clause can come from common blocks ?!

I am not sure if I completely understand your question.
struct OmpObject wraps over std::variant<Designator, /*common block*/ Name> u, and if you were trying to ask why did we change that from Designator to OmpObject check comments at https://reviews.llvm.org/D93105#inline-870822.

This revision was landed with ongoing or failed builds.Dec 22 2020, 9:05 AM
This revision was automatically updated to reflect the committed changes.
sameeranjoshi edited the summary of this revision. (Show Details)Dec 22 2020, 9:25 AM

The co-authorship must be in the commit message. Updating phab doesn't change what was landed. Look at https://github.com/llvm/llvm-project/commit/8a58f21f5b6c228137a9b87906fe5b720c4d1dfb, the co-authorship is correctly displayed on GitHub. This is not the case with the commit you landed for this revision.

clementval reopened this revision.Dec 22 2020, 12:23 PM

Reopen the revision since it was reverted and other patches depend on it.

This revision is now accepted and ready to land.Dec 22 2020, 12:23 PM
This revision was automatically updated to reflect the committed changes.