Page MenuHomePhabricator

[flang][OpenMP] Add semantic check for close nesting of `master` regions
AcceptedPublic

Authored by arnamoy10 on Apr 9 2021, 1:27 PM.

Details

Summary

This patch implements the following semantic check:

A master region may not be closely nested inside a work-sharing, loop, atomic, task, or taskloop region.

Adds a test case and also modifies a couple of existing test cases to include the check.

Diff Detail

Event Timeline

arnamoy10 created this revision.Apr 9 2021, 1:27 PM
arnamoy10 requested review of this revision.Apr 9 2021, 1:27 PM
Herald added a project: Restricted Project. · View Herald Transcript
kiranchandramohan requested changes to this revision.Apr 10 2021, 3:19 AM
kiranchandramohan added inline comments.
flang/lib/Semantics/check-omp-structure.cpp
504

Nit: Can we check whether it is the master construct before calling the check function?

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

Not for this patch. It will be good to generate these Sets from the OMP.td file.

92

Why is parallelSet here? Master is allowed inside parallel.

This revision now requires changes to proceed.Apr 10 2021, 3:19 AM

Addressing reviewers comments.

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

Done, thanks!

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

Good catch, updated the code and removed it. Now the set is inspired from here

kiranchandramohan requested changes to this revision.Fri, Apr 23, 11:08 AM
kiranchandramohan added inline comments.
flang/lib/Semantics/check-omp-structure.h
92

Can you add all those combined constructs also. Those combined constructs which will lead to a nested master violating this constraint, like parallel do.

This revision now requires changes to proceed.Fri, Apr 23, 11:08 AM
arnamoy10 added inline comments.Mon, Apr 26, 1:31 PM
flang/lib/Semantics/check-omp-structure.h
92

Can you please review this patch? Then I should be able to use the combined constructs for worksharing from the patch here.

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

Apologies, I missed it completely.

arnamoy10 updated this revision to Diff 343485.Thu, May 6, 1:09 PM

Updating the patch to use the recent merged workShareSet (that included tests for combined constructs for worksharing loops).

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

Is a master construct not allowed in a master construct?

arnamoy10 updated this revision to Diff 343682.Fri, May 7, 7:57 AM

Removing master construct from the error set.

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

Thanks for the catch, just updated the code.

This revision is now accepted and ready to land.Fri, May 7, 2:34 PM