Page MenuHomePhabricator

[flang][OpenMP][FIX] Fix the worksharing nesting check with inclusion of more constructs to cover entire workshare family.
ClosedPublic

Authored by arnamoy10 on Thu, Apr 15, 9:17 AM.

Details

Summary

Existing code for checking the nesting of workshare regions was not covering all the possible constructs that can be workshare constructs. This patch creates a new group of constructs for workshare constructs.

It extends the check to check during sections constructs as well.

Also adds a few test cases to verify the check.

Diff Detail

Event Timeline

arnamoy10 created this revision.Thu, Apr 15, 9:17 AM
arnamoy10 requested review of this revision.Thu, Apr 15, 9:17 AM
Herald added a project: Restricted Project. · View Herald Transcript
kiranchandramohan requested changes to this revision.Mon, Apr 26, 2:51 PM
kiranchandramohan added inline comments.
flang/lib/Semantics/check-omp-structure.h
44–45

Not sure why parallel is here. Remove if no test fails.

71–75

Why do a few of these have the llvm::omp prefix but others do not?

76

Nit: empty line.

This revision now requires changes to proceed.Mon, Apr 26, 2:51 PM
arnamoy10 updated this revision to Diff 340959.Tue, Apr 27, 1:00 PM

Addressing comments.

flang/lib/Semantics/check-omp-structure.h
44–45

Done, thanks

71–75

typo, thanks for the catch

LGTM. Have two Nit comments.

flang/lib/Semantics/check-omp-structure.cpp
363–371

Nit: Can a call to HasInvalidWorksharingNesting be added here for the worksharing construct with name workshare? And add one test.

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

Nit: Can you construct this set using the doSet?

This revision is now accepted and ready to land.Wed, Apr 28, 3:13 PM
arnamoy10 updated this revision to Diff 341481.Thu, Apr 29, 5:26 AM

Addressing comments.

flang/lib/Semantics/check-omp-structure.cpp
363–371

Sure, done and added test case

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

Thanks, done!

arnamoy10 closed this revision.Thu, Apr 29, 1:17 PM