This is an archive of the discontinued LLVM Phabricator instance.

[flang][OpenMP] Add semantic check for occurrence of constructs nested inside a SIMD region
ClosedPublic

Authored by arnamoy10 on Apr 1 2021, 11:39 AM.

Details

Summary

This patch adds the semantic check for occurrence of constructs nested inside a SIMD region.

To make the test generalized for multiple constructs, to avoid repeats, the check is done while Enter()-ing the top level OpenMPConstruct

Also adds a test case.

It also modifies an existing test case for nested ordered clause check because with the current check introduced, the previous test shows two sema check failures instead of one.

PS: The check is currently supporting SIMD loops, but not declare SIMD directive.

Diff Detail

Event Timeline

arnamoy10 created this revision.Apr 1 2021, 11:39 AM
arnamoy10 requested review of this revision.Apr 1 2021, 11:39 AM
Herald added a project: Restricted Project. · View Herald Transcript
arnamoy10 edited the summary of this revision. (Show Details)Apr 1 2021, 11:40 AM
clementval added inline comments.Apr 1 2021, 8:39 PM
flang/lib/Semantics/check-omp-structure.cpp
247

It is also considered nested is it's bigger than 1. So it should be >= 1?

368

Apply clang-format please.

arnamoy10 updated this revision to Diff 335241.Apr 5 2021, 6:06 AM
  1. Modifying the if-check to trigger the test.
  2. Clang-formatting.
flang/lib/Semantics/check-omp-structure.cpp
365

Nit: use braced initializers.

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

Which revision of the standard are you following here?

4.5:
An ordered construct with the simd clause is the only OpenMP construct that can be encountered during execution of a simd region.

5.0, 5.1:
The only OpenMP constructs that can be encountered during execution of a simd region are the atomic construct, the loop construct, the simd construct and the ordered construct with the simd clause.

arnamoy10 updated this revision to Diff 336507.Apr 9 2021, 10:06 AM

Updating the checks (and test cases) to be compliant with the 5.0/5.1 standard.

arnamoy10 added inline comments.Apr 9 2021, 10:07 AM
flang/test/Semantics/omp-nested-simd.f90
76

Thanks for the comment. Initially the check I implemented was based on gfortran (latest) error messages. I updated the checks to reflect the OpenMP 5.0/5.1 standard.

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

Can you check for the SIMD construct before calling this function?

370

Nit: use braced initializer.

371

Will simd be always the parent construct? Are there cases where it can become an ancestor but not the immediate parent?

kiranchandramohan requested changes to this revision.Apr 11 2021, 4:16 PM
This revision now requires changes to proceed.Apr 11 2021, 4:16 PM
arnamoy10 updated this revision to Diff 337542.Apr 14 2021, 1:46 PM

Addressing review comment to check for SIMD construct in the whole context chain instead of only searching at the immediate parent level.

Also added a test accordingly.

kiranchandramohan requested changes to this revision.Apr 25 2021, 7:05 AM

Have a question here.

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

Nit: A TODO for the loop construct.

378

Here ordered is a block-directive. But below it is a standalone directive. Is it parsed both ways?

392

Here ordered is a standalone directive. See above.

This revision now requires changes to proceed.Apr 25 2021, 7:05 AM
arnamoy10 updated this revision to Diff 341246.Apr 28 2021, 9:46 AM

Small change in function docstring as per reviewers suggestion.

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

Thanks, added.

378

Yes, according to here,

If the depend clause is specified, the ordered construct is a stand-alone directive.

LGTM. I have a Nit comment about a counter. Leaving the decision to you on whether to consider it or not.

flang/lib/Semantics/check-directive-structure.h
230

Nit: gven -> given

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

Nit: Would it be better to keep a counter which counts the constructs in the simdSet? The counter can be incremented on Entering a simdSet construct and decremented on leaving. If counter is greater than 1 then FindDirInWholeContext is true. This way we avoid going through all the entries in dirContext_ everytime we see a construct.

368

Nit: not -> note

This revision is now accepted and ready to land.Apr 29 2021, 4:12 PM
arnamoy10 updated this revision to Diff 343150.May 5 2021, 12:49 PM

Using a variable to identify SIMD nesting as per suggestion to avoid expensive traversals.

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

Thanks for the suggestion. Updated

kiranchandramohan accepted this revision.May 5 2021, 2:45 PM

LGTM.

flang/lib/Semantics/check-directive-structure.h
323

Nit: Braced initialization.