This is an archive of the discontinued LLVM Phabricator instance.

[flang][OpenMP] Add semantic check for teams nesting
ClosedPublic

Authored by peixin on Jul 19 2021, 7:52 PM.

Details

Summary

This patch implements the following check for TEAMS construct:

OpenMP Version 5.0 Teams construct restriction: A teams region can
only be strictly nested within the implicit parallel region or a target
region. If a teams construct is nested within a target construct, that
target construct must contain no statements, declarations or directives
outside of the teams construct.

Also add one test case for the check.

Diff Detail

Event Timeline

peixin created this revision.Jul 19 2021, 7:52 PM
peixin requested review of this revision.Jul 19 2021, 7:52 PM
peixin updated this revision to Diff 360004.Jul 19 2021, 9:00 PM

Fix errors info in related test cases.

peixin updated this revision to Diff 360006.Jul 19 2021, 9:01 PM
peixin edited the summary of this revision. (Show Details)Jul 20 2021, 12:18 AM
clementval requested changes to this revision.Jul 20 2021, 4:20 AM
clementval added inline comments.
flang/lib/Semantics/check-directive-structure.h
212

This is only for OpenMP so it should be moved to check-omp-structure.h. Same for the two functions below. The check-directive-structure.h is meant to be shared bewteen OpenMP and OpenACC.

flang/test/Semantics/omp-firstprivate01.f90
15

Which statement is outside?

flang/test/Semantics/omp-nested-master.f90
90

parallel region instead of parallelregion

106

Missing a space here as well.

122

Missing space

139

Missing space

flang/test/Semantics/omp-nested-simd.f90
45

Missing space

This revision now requires changes to proceed.Jul 20 2021, 4:20 AM
peixin updated this revision to Diff 360138.Jul 20 2021, 8:04 AM

@clementval Thanks for the review.

Moved two sets of functions and counters to check-omp-structure.h and merged them. Also fix the errors info in the test cases.

peixin added inline comments.Jul 20 2021, 11:24 PM
flang/test/Semantics/omp-firstprivate01.f90
15

This target directive has two teams construct nested inside and both of teams construct contain statements. Change the error info by replacing statements by statements or directives, which should be more reasonable.

Looks OK. Have two questions.

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

Is the parent teams check required?

flang/lib/Semantics/check-omp-structure.h
255

Nit: Can we add a LastType at the end of the enum and use it as the size of the directiveNest_ array?

peixin updated this revision to Diff 362659.Jul 29 2021, 12:27 AM
peixin added a reviewer: Chuanfeng.
peixin set the repository for this revision to rG LLVM Github Monorepo.

@kiranchandramohan Thanks for the review. Fixed the patch according to your comments.

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

The restriction for two strictly nested teams has already been checked with the error info of distribute and parallel region only allowed strictly nested inside teams region. Rethink about it and reporting one more error here is more reasonable. Deleted the check of parent teams and added the error info in test case omp-nested-teams.f90.

flang/lib/Semantics/check-omp-structure.h
255

Good idea. Added it.

peixin updated this revision to Diff 362666.Jul 29 2021, 1:24 AM

Change directiveNest_[2] to directiveNest_[3] since one more element LastType was added.

LGTM.

@clementval do you have further comments?

flang/lib/Semantics/check-omp-structure.h
254

Nit: Sorry, I meant can you move this below the enum and use LastType (with SIMDNest =1 ) or LastType +1 as the size of the directiveNest_array?

enum directiveNestType { SIMDNest, TargetBlockOnlyTeams, LastType };
int directiveNest_[LastType+1] = {0};
peixin updated this revision to Diff 362715.Jul 29 2021, 4:22 AM
peixin set the repository for this revision to rG LLVM Github Monorepo.

@kiranchandramohan Thanks for the suggestions, which prevents from using magic number. Replaced directiveNest_[3] with directiveNest_[LastType+1].

clementval accepted this revision.Aug 12 2021, 7:12 PM

LGTM as well. Sorry for the delay in reviewing the change to the patch. You can go ahead and land it.

This revision is now accepted and ready to land.Aug 12 2021, 7:12 PM
This revision was automatically updated to reflect the committed changes.