Page MenuHomePhabricator

[Flang][Openmp] Upgrade TASKGROUP construct to 5.0.
Needs ReviewPublic

Authored by sameeranjoshi on Dec 15 2020, 11:00 PM.

Details

Summary

In OMP 5.0 specification clause-list with

  • task_reduction
  • allocate

were allowed on taskgroup construct.

Fix XFAIL - omp-taskloop01.f90.

Depends on D93105 for task_reduction clause.

Diff Detail

Event Timeline

sameeranjoshi created this revision.Dec 15 2020, 11:00 PM
sameeranjoshi requested review of this revision.Dec 15 2020, 11:00 PM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a subscriber: sstefan1. · View Herald Transcript

Fix newlines.

sameeranjoshi added a project: Restricted Project.Dec 15 2020, 11:17 PM
clementval added inline comments.Dec 16 2020, 6:47 AM
flang/lib/Lower/OpenMP.cpp
242

If you add the todo can you maybe explicitly add an error message in it?

flang/test/Semantics/omp-taskgroup01.f90
36

Why is this block commented out?

sameeranjoshi added inline comments.Dec 16 2020, 10:27 AM
flang/lib/Lower/OpenMP.cpp
242

What in case we remove it, that's ok is it?

flang/test/Semantics/omp-taskgroup01.f90
36

Depends on D93105

clementval added inline comments.Dec 16 2020, 10:58 AM
flang/lib/Lower/OpenMP.cpp
242

Fine as well. In the current state it doesn't add more.

flang/test/Semantics/omp-taskgroup01.f90
36

Yeah but since you have it as a dependency in phab you can uncomment the block.

clementval added inline comments.Dec 16 2020, 4:34 PM
flang/test/Semantics/omp-taskgroup01.f90
36

What I mean is that the pre-merge check for example will apply the patch you depend on before applying this one so you will not have failure.

Address review comments.

flang/test/Semantics/omp-taskgroup01.f90
40

A colon (not comma) is expected right?

flang/test/Semantics/omp-taskgroup01.f90
38

reduction_var is not declared?

49

Should this be taskgroup or is this accidentally added?

52

Check count of begin and end taskgroups.

Summary of changes:

  • Simplify the test as those checks were already covered.
  • Update OMP.td to specifiy to add 5.0 standard for the clauses.

Rebase after task_reduction(D93105).

sameeranjoshi marked 7 inline comments as done.Jan 12 2021, 11:47 AM

These are great catches, thank you.
I was wondering since the merge of task_reduction why were the tests failing and did kept this on hold for a while since then.
Thanks again. :)

flang/test/Semantics/omp-taskgroup01.f90
40

Ideally it was supposed to be caught in parser itself, unfortunately it didn't.
May be this needs to be investigated in new patch.

sameeranjoshi marked an inline comment as done.Jan 19 2021, 9:41 AM

Ping.

llvm/include/llvm/Frontend/OpenMP/OMP.td
443 ↗(On Diff #316180)

Are tests failing due to task_reduction/allocate marked as version 5.0?

sameeranjoshi added inline comments.Jan 19 2021, 10:34 AM
llvm/include/llvm/Frontend/OpenMP/OMP.td
443 ↗(On Diff #316180)

All clang related OpenMP tests are failing which contain flag -fopenmp-version=45 for compilation along with -fopenmp.
The difference of 4.5 and 5.0 specifically mentions these clauses to in 5.0 standard which was the reason to add 50.
Should we remove this string do is there some other place to change in TableGen?

kiranchandramohan added inline comments.
llvm/include/llvm/Frontend/OpenMP/OMP.td
443 ↗(On Diff #316180)

I think in general for these situation we have chosen not to add the 5.0 version. Is that right @clementval?

But correcting these testcases is probably the right thing to do. But for that we might need some inputs from @jdoerfert or @Meinersbur.

clementval added inline comments.Jan 19 2021, 11:26 AM
llvm/include/llvm/Frontend/OpenMP/OMP.td
443 ↗(On Diff #316180)

Flang is not using this information now but clang is using it. So if you introduce the version on those clauses and some clang/openmp tests are failing they should be investigated.