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

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

Details

Reviewers

kiranchandramohan
clementval
kiranktp
praveen
jdoerfert

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

Repository: rG LLVM Github Monorepo

Unit TestsFailed
View All

	Time	Test
	80 ms	x64 debian > Clang.OpenMP::master_taskloop_simd_ast_print.cpp Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/bin/clang -cc1 -internal-isystem /mnt/disks/ssd0/agent/llvm-project/build/lib/clang/12.0.0/include -nostdsysteminc -verify -fopenmp -fopenmp-version=45 -ast-print /mnt/disks/ssd0/agent/llvm-project/clang/test/OpenMP/master_taskloop_simd_ast_print.cpp \| /mnt/disks/ssd0/agent/llvm-project/build/bin/FileCheck /mnt/disks/ssd0/agent/llvm-project/clang/test/OpenMP/master_taskloop_simd_ast_print.cpp --check-prefix CHECK --check-prefix OMP45
	80 ms	x64 debian > Clang.OpenMP::parallel_master_taskloop_simd_ast_print.cpp Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/bin/clang -cc1 -internal-isystem /mnt/disks/ssd0/agent/llvm-project/build/lib/clang/12.0.0/include -nostdsysteminc -verify -fopenmp -fopenmp-version=45 -ast-print /mnt/disks/ssd0/agent/llvm-project/clang/test/OpenMP/parallel_master_taskloop_simd_ast_print.cpp \| /mnt/disks/ssd0/agent/llvm-project/build/bin/FileCheck /mnt/disks/ssd0/agent/llvm-project/clang/test/OpenMP/parallel_master_taskloop_simd_ast_print.cpp --check-prefix CHECK --check-prefix OMP45
	80 ms	x64 debian > Clang.OpenMP::taskloop_simd_ast_print.cpp Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/bin/clang -cc1 -internal-isystem /mnt/disks/ssd0/agent/llvm-project/build/lib/clang/12.0.0/include -nostdsysteminc -verify -fopenmp -fopenmp-version=45 -ast-print /mnt/disks/ssd0/agent/llvm-project/clang/test/OpenMP/taskloop_simd_ast_print.cpp \| /mnt/disks/ssd0/agent/llvm-project/build/bin/FileCheck /mnt/disks/ssd0/agent/llvm-project/clang/test/OpenMP/taskloop_simd_ast_print.cpp --check-prefix CHECK --check-prefix OMP45
	100 ms	x64 windows > Clang.OpenMP::master_taskloop_simd_ast_print.cpp Script: -- : 'RUN: at line 1'; c:\ws\w32-1\llvm-project\premerge-checks\build\bin\clang.exe -cc1 -internal-isystem c:\ws\w32-1\llvm-project\premerge-checks\build\lib\clang\12.0.0\include -nostdsysteminc -verify -fopenmp -fopenmp-version=45 -ast-print C:\ws\w32-1\llvm-project\premerge-checks\clang\test\OpenMP\master_taskloop_simd_ast_print.cpp \| c:\ws\w32-1\llvm-project\premerge-checks\build\bin\filecheck.exe C:\ws\w32-1\llvm-project\premerge-checks\clang\test\OpenMP\master_taskloop_simd_ast_print.cpp --check-prefix CHECK --check-prefix OMP45
	100 ms	x64 windows > Clang.OpenMP::parallel_master_taskloop_simd_ast_print.cpp Script: -- : 'RUN: at line 1'; c:\ws\w32-1\llvm-project\premerge-checks\build\bin\clang.exe -cc1 -internal-isystem c:\ws\w32-1\llvm-project\premerge-checks\build\lib\clang\12.0.0\include -nostdsysteminc -verify -fopenmp -fopenmp-version=45 -ast-print C:\ws\w32-1\llvm-project\premerge-checks\clang\test\OpenMP\parallel_master_taskloop_simd_ast_print.cpp \| c:\ws\w32-1\llvm-project\premerge-checks\build\bin\filecheck.exe C:\ws\w32-1\llvm-project\premerge-checks\clang\test\OpenMP\parallel_master_taskloop_simd_ast_print.cpp --check-prefix CHECK --check-prefix OMP45
		View Full Test Results (6 Failed)

Event Timeline

sameeranjoshi created this revision.Dec 15 2020, 11:00 PM

Herald added subscribers: mehdi_amini, guansong, yaxunl. · View Herald TranscriptDec 15 2020, 11:00 PM

sameeranjoshi requested review of this revision.Dec 15 2020, 11:00 PM

Herald added a reviewer: jdoerfert. · View Herald TranscriptDec 15 2020, 11:00 PM

Herald added a project: Restricted Project. · View Herald Transcript

Herald added a subscriber: sstefan1. · View Herald Transcript

Harbormaster completed remote builds in B82585: Diff 312118.Dec 15 2020, 11:02 PM

Fix newlines.

Harbormaster completed remote builds in B82586: Diff 312119.Dec 15 2020, 11:13 PM

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 ↗	(On Diff #312119)	If you add the todo can you maybe explicitly add an error message in it?
flang/test/Semantics/omp-taskgroup01.f90
37	Why is this block commented out?

sameeranjoshi added inline comments.Dec 16 2020, 10:27 AM

flang/lib/Lower/OpenMP.cpp
242 ↗	(On Diff #312119)	What in case we remove it, that's ok is it?
flang/test/Semantics/omp-taskgroup01.f90
37	Depends on D93105

clementval added inline comments.Dec 16 2020, 10:58 AM

flang/lib/Lower/OpenMP.cpp
242 ↗	(On Diff #312119)	Fine as well. In the current state it doesn't add more.
flang/test/Semantics/omp-taskgroup01.f90
37	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
37	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.

Harbormaster completed remote builds in B82765: Diff 312436.Dec 17 2020, 3:53 AM

kiranchandramohan added inline comments.Jan 11 2021, 3:52 PM

flang/test/Semantics/omp-taskgroup01.f90
40	A colon (not comma) is expected right?

kiranchandramohan added inline comments.Jan 11 2021, 4:02 PM

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

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.

Harbormaster completed remote builds in B84888: Diff 316180.Jan 12 2021, 12:36 PM

Ping.

kiranchandramohan added inline comments.Jan 19 2021, 10:13 AM

llvm/include/llvm/Frontend/OpenMP/OMP.td
443	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	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 a subscriber: Meinersbur.Jan 19 2021, 11:11 AM

kiranchandramohan added inline comments.

llvm/include/llvm/Frontend/OpenMP/OMP.td
443	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	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.