[flang][OpenMP] Added parser support for defaultmap (OpenMP 5.0)

[do]

Summary

This patch adds parser support for defaultmap clause [OpenMP 5.0].

[kiranchandramohan]

Thanks @do for this patch. I have two comments.

1. I think the linux CI is failing since the patch is not formatted. Can you run clang-format on the two source files (flang/include/flang/Parser/parse-tree.h, flang/lib/Parser/openmp-parsers.cpp)? Since we build clang also along with flang you likely have clang-format as well in your bin.

./bin/clang-format -i <path-to-filename>

2. Can you also add a parse-tree dump test similar to what is in the following file? This is just to check that the parse-tree created is correct and it is easier for the reviewers to see the parse-tree.

https://github.com/llvm/llvm-project/blob/main/flang/test/Parser/omp-sections.f90

[do]

In D124190#3471545, @kiranchandramohan wrote:

Thanks @do for this patch. I have two comments.

1. I think the linux CI is failing since the patch is not formatted. Can you run clang-format on the two source files (flang/include/flang/Parser/parse-tree.h, flang/lib/Parser/openmp-parsers.cpp)? Since we build clang also along with flang you likely have clang-format as well in your bin.

./bin/clang-format -i <path-to-filename>

2. Can you also add a parse-tree dump test similar to what is in the following file? This is just to check that the parse-tree created is correct and it is easier for the reviewers to see the parse-tree.

https://github.com/llvm/llvm-project/blob/main/flang/test/Parser/omp-sections.f90

Thanks @kiranchandramohan! Both are addressed.

[peixin]

CI fails. Can you check it?

[peixin]

flang/include/flang/Parser/parse-tree.h

3346 ↗

(On Diff #425020)

The comment needs changes since you are extending 4.5 to 5.0.

3351 ↗

(On Diff #425020)

I guess the CI fails for the test case since Aggregate cannot be recognized. Please check it.

[do]

In D124190#3473472, @peixin wrote:

CI fails. Can you check it?

thanks @peixin! will do.

[do]

Addressed comments. Should fix the CI failures as well.

[do]

flang/include/flang/Parser/parse-tree.h

3346 ↗

(On Diff #425020)

will do. thanks.

3351 ↗

(On Diff #425020)

will do. thanks.

[peixin]

flang/lib/Parser/openmp-parsers.cpp

54–59

Please remove this previous comment.

58

[do]

flang/lib/Parser/openmp-parsers.cpp

54–59

Will do. Thanks.

[do]

addressing comments.

[kiranchandramohan]

Thanks Dossay for the contribution. LGTM with the two Nits suggested and CI passing. Please wait for @shraiysh and @peixin to approve.

flang/test/Parser/omp-defaultmap-unparse.f90

35–56 ↗

(On Diff #425719)

Nit: For the unparse tests only the relevant section is required. You can remove the checks for the rest from all the tests.
For e.g for this test, the relevant section is the following.

!PARSE-TREE:      OmpBeginBlockDirective
!PARSE-TREE:        OmpBlockDirective -> llvm::omp::Directive = target
!PARSE-TREE:        OmpClauseList -> OmpClause -> Defaultmap -> OmpDefaultmapClause
!PARSE-TREE:          ImplicitBehavior = Tofrom
!PARSE-TREE:          VariableCategory = Scalar

254–279 ↗

(On Diff #425719)

Nit: I think you can move these entries also close to the individual tests.

[do]

In D124190#3479479, @kiranchandramohan wrote:

Thanks Dossay for the contribution. LGTM with the two Nits suggested and CI passing. Please wait for @shraiysh and @peixin to approve.

Thanks Kiran! Done.

[kiranchandramohan]

flang/test/Parser/omp-defaultmap-unparse.f90

42–55 ↗

(On Diff #425737)

@do I meant you can remove the check for Block and OmpEndBlockDirective and just have the following.

!PARSE-TREE:      OmpBeginBlockDirective
!PARSE-TREE:        OmpBlockDirective -> llvm::omp::Directive = target
!PARSE-TREE:        OmpClauseList -> OmpClause -> Defaultmap -> OmpDefaultmapClause
!PARSE-TREE:          ImplicitBehavior = Tofrom
!PARSE-TREE:          VariableCategory = Scalar

[peixin]

flang/include/flang/Parser/parse-tree.h

3346 ↗

(On Diff #425020)

This change is missed.

[do]

addressing comments.

[shraiysh]

LGTM, thank you for working on this and apologies for the delay in review.

[peixin]

Is this landed?

[do]

In D124190#3518117, @peixin wrote:

Is this landed?

Thanks @peixin for checking, working on it. Will update when done.

[shraiysh]

@do I am trying to land this patch for you as you suggested, but I need your email address to make you the author of this patch. I believe this patch was uploaded via the web interface and so it doesn't have your email address tied to it.

flang/lib/Parser/openmp-parsers.cpp

("TO"_id >> pure(OmpMapType::Type::To) ||
         "TOFROM" >> pure(OmpMapType::Type::Tofrom) ||
         "ALLOC" >> pure(OmpMapType::Type::Alloc) ||
         "RELEASE" >> pure(OmpMapType::Type::Release) ||
         "DELETE" >> pure(OmpMapType::Type::Delete)) /
         ":"))
 
 TYPE_PARSER(construct<OmpMapClause>(
     maybe(Parser<OmpMapType>{}), Parser<OmpObjectList>{}))
 
-// 2.15.5.2 defaultmap -> DEFAULTMAP (TOFROM:SCALAR)
 // [OpenMP 5.0]
 // 2.19.7.2 defaultmap(implicit-behavior[:variable-category])
-//          implicit-behavior -> ALLOC | TO | FROM | TOFROM | FIRSRTPRIVATE |
-//          NONE | DEFAULT variable-category -> SCALAR | AGGREGATE | ALLOCATABLE
-//          | POINTER
+//  implicit-behavior -> ALLOC | TO | FROM | TOFROM | FIRSRTPRIVATE | NONE |
+//  DEFAULT
+//  variable-category -> SCALAR | AGGREGATE | ALLOCATABLE | POINTER
 TYPE_PARSER(construct<OmpDefaultmapClause>(
     construct<OmpDefaultmapClause::ImplicitBehavior>(
         "ALLOC" >> pure(OmpDefaultmapClause::ImplicitBehavior::Alloc) ||
         "TO"_id >> pure(OmpDefaultmapClause::ImplicitBehavior::To) ||
         "FROM" >> pure(OmpDefaultmapClause::ImplicitBehavior::From) ||
         "TOFROM" >> pure(OmpDefaultmapClause::ImplicitBehavior::Tofrom) ||
         "FIRSTPRIVATE" >>
             pure(OmpDefaultmapClause::ImplicitBehavior::Firstprivate) ||