Page MenuHomePhabricator

[Flang][OpenMP 4.5] Add semantic check for OpenMP shared and private clause
ClosedPublic

Authored by yhegde on Oct 14 2020, 7:59 AM.

Details

Summary

Semantic check for OpenMP 4.5 - 2.15.3.2 and 2.15.3.3 shared and private clause

Test cases : omp-parallel-shared.f90

        omp-parallel-shared02.f90
        omp-parallel-shared03.f90
        omp-parallel-shared04.f90    
and  omp-parallel-private.f90
        omp-parallel-private02.f90
        omp-parallel-private03.f90
        omp-parallel-private04.f90

Diff Detail

Event Timeline

yhegde created this revision.Oct 14 2020, 7:59 AM
yhegde requested review of this revision.Oct 14 2020, 7:59 AM
yhegde added a project: Restricted Project.Oct 14 2020, 8:01 AM
clementval requested changes to this revision.Oct 14 2020, 9:13 AM
clementval added a subscriber: clementval.
clementval added inline comments.
flang/lib/Semantics/check-directive-structure.h
61 ↗(On Diff #298153)

Since this is OpenMP specific, it should be in check-omp-structure and not here.

66 ↗(On Diff #298153)

Same here. Should be moved to check-omp-structure.h

This revision now requires changes to proceed.Oct 14 2020, 9:13 AM
clementval added inline comments.Oct 14 2020, 9:46 AM
flang/lib/Semantics/check-directive-structure.h
199 ↗(On Diff #298153)

OmpObjectList is specific to OpenMP and doesn't fit in check-directive-structure.h

238 ↗(On Diff #298153)

In the rest of Flang there is no blank line between each specific visitor so you might want to stick with the same coding style.

tskeith added inline comments.
flang/lib/Semantics/check-directive-structure.h
212 ↗(On Diff #298153)

This code just seems to be trying to determine if ompObject is a StructureComponent. If so, it could be simplified to:

if (parser::Unwrap<parser::StructureComponent>(ompObject)) {
yhegde updated this revision to Diff 298335.Oct 15 2020, 2:34 AM

Thanks for this patch. Did you miss the array element check?

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

Nit: you can skip the standard section number here.

413

Nit: you can skip the standard section number here.

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

Nit: You can skip the section number here.

flang/test/Semantics/omp-parallel-private.f90
8 ↗(On Diff #298335)

Nit: alignment

14–17 ↗(On Diff #298335)

Is this loop required for this test?

21–24 ↗(On Diff #298335)

Nit: Alignment.

flang/test/Semantics/omp-parallel-shared.f90
8 ↗(On Diff #298335)

Nit: alignment

11–15 ↗(On Diff #298335)

Is this loop required for this test?

yhegde updated this revision to Diff 299059.Oct 19 2020, 8:09 AM
yhegde edited the summary of this revision. (Show Details)
  1. Array element check added along with test cases to test the same.
yhegde updated this revision to Diff 299106.Oct 19 2020, 11:02 AM

clang format

This comment was removed by Prashanth.
clementval added inline comments.Oct 28 2020, 7:39 AM
flang/lib/Semantics/check-omp-structure.cpp
427

Clause names are usually uppercase in the error message.

LGTM.
Please wait for approval from @clementval after uppercasing the clause names.

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

Nit: Would a better name be CheckIsVarPartOfAnotherVar?

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

Nit: Can this be private?

yhegde updated this revision to Diff 303037.Wed, Nov 4, 10:02 PM
  1. Function name changed to CheckIsVarPartOfAnotherVar
  2. Clause names in uppercase.
clementval accepted this revision.Thu, Nov 5, 7:18 AM

Ok from my side. Maybe @kiranchandramohan wants to double check the latest changes.

This revision is now accepted and ready to land.Thu, Nov 5, 7:18 AM

@yhegde please submit. If you require any inputs then please let me know. This will be helpful for D91159.

yhegde updated this revision to Diff 304899.Thu, Nov 12, 10:55 AM
yhegde added a reviewer: sameeranjoshi.

Thanks a lot for the review comments.
Requesting a quick review of this accepted patch. I had to revert the code part related to Shared and Private clause from the review https://reviews.llvm.org/D90324.

Test cases : omp-parallel-shared.01f90

        omp-parallel-shared02.f90
        omp-parallel-shared03.f90
        omp-parallel-shared04.f90    
and  omp-parallel-private.01f90
        omp-parallel-private02.f90
        omp-parallel-private03.f90
        omp-parallel-private04.f90

Shall I assume there are no issues with this patch. ?

No issues, please go ahead.