[PATCH] D89562: [flang]Add Parser Support for OpenMP Allocate Directive

[sameeran joshi via Phabricator via llvm-commits]

sameeranjoshi added a comment.

Thanks for working on this, I have mostly a few queries and some suggestions.



================
Comment at: flang/include/flang/Parser/parse-tree.h:3619
+// 2.11.3 allocate -> ALLOCATE [(variable-name-list)] [clause]
+//        allocate-statement
+//        clause -> allocator-clause
----------------
Are you missing `OpenMPDeclarativeAllocate` in comments?


================
Comment at: flang/include/flang/Parser/parse-tree.h:3625
+  std::tuple<Verbatim, std::optional<OmpObjectList>, OmpClauseList,
+      std::optional<OpenMPDeclarativeAllocate>, Statement<AllocateStmt>>
+      t;
----------------
Correct me if my interpretation from standard is wrong.
```
!$omp allocate[(list)] clause
[!$omp allocate(list)clause[...]]
allocate statement
```
the 
`std::optional<OpenMPDeclarativeAllocate>` seem to be a list mentioned by elipsis, or is it that the there should be a list of clauses?

Althought there are tests(`omp-allocate-directive.f90` line 19-23) which have a list of `OpenMPDeclarativeAllocate` for an `OpenMPExecutableAllocate ` test.


================
Comment at: flang/lib/Parser/openmp-parsers.cpp:457
+    sourced(construct<OpenMPExecutableAllocate>(verbatim("ALLOCATE"_tok),
+        maybe(parenthesized(Parser<OmpObjectList>{})), Parser<OmpClauseList>{},
+        maybe(Parser<OpenMPDeclarativeAllocate>{}) / endOmpLine,
----------------
In case of `OpenMPExecutableAllocate` are the clauses mandatory as the standard doesn't seem to have square brackets around them?
Or that's wrongly mentioned in standard.

As `OmpClauseList` is a list it can be optional.
```
!$omp allocate[(list)] clause
[!$omp allocate[(list)] clause
[...]]
```

So a test case like
```
  !$omp allocate(a, b)
     allocate ( darray(a, b) )
```
becomes invalid in that case.


================
Comment at: flang/lib/Parser/openmp-parsers.cpp:474
+        parenthesized(Parser<OmpObjectList>{}), Parser<OmpClauseList>{})) /
+    lookAhead(endOmpLine / !statement(allocateStmt)))
+
----------------
Does a test with no newline and an allocateStmt fall under `OpenMPDeclarativeAllocate`  as per current implementations?
```
!$omp allocate(x, y) allocator(omp_default_mem_alloc) allocate( darray(x,y) )
```


================
Comment at: flang/lib/Parser/unparse.cpp:2296
+    Put("\n");
+    EndOpenMP();
+    Walk(std::get<Statement<AllocateStmt>>(x.t));
----------------
Why is there no `Walk` for `std::optional<OpenMPDeclarativeAllocate>` as parse-tree.h shows
```
   std::tuple<Verbatim, std::optional<OmpObjectList>, OmpClauseList,
       std::optional<OpenMPDeclarativeAllocate>, Statement<AllocateStmt>>
       t;
``` 


================
Comment at: flang/lib/Semantics/check-omp-structure.h:107
   void Leave(const parser::OpenMPDeclareSimdConstruct &);
+  void Enter(const parser::OpenMPDeclarativeAllocate &);
+  void Leave(const parser::OpenMPDeclarativeAllocate &);
----------------
The commit title mentions this to be a parser change, are these semantic checks necessary with this patch?


================
Comment at: flang/test/Parser/omp-allocate-unparse.f90:7
+
+real, dimension (:,:), allocatable :: darray    
+integer :: a, b, m, n, t, x, y, z
----------------
I see whitespaces at the end.


================
Comment at: flang/test/Parser/omp-allocate-unparse.f90:18
+!$omp allocate(a, b)
+    allocate ( darray(a, b) ) 
+!$omp allocate allocator(omp_default_mem_alloc)
----------------
whitespace


================
Comment at: flang/test/Parser/omp-allocate-unparse.f90:20
+!$omp allocate allocator(omp_default_mem_alloc)
+    allocate ( darray(a, b) ) 
+!$omp allocate(a, b) allocator(omp_default_mem_alloc)
----------------
whitespace.


================
Comment at: flang/test/Parser/omp-allocate-unparse.f90:22
+!$omp allocate(a, b) allocator(omp_default_mem_alloc)
+    allocate ( darray(a, b) ) 
+
----------------
whitespace


================
Comment at: flang/test/Parser/omp-allocate-unparse.f90:28
+!$omp allocate(n)
+    allocate ( darray(z, t) ) 
+
----------------
whitespace


================
Comment at: flang/test/Semantics/omp-allocate-directive.f90:8
+
+real, dimension (:,:), allocatable :: darray    
+integer :: a, b, x, y, m, n, t, z
----------------
whitespace at end.


================
Comment at: flang/test/Semantics/omp-allocate-directive.f90:14
+!$omp allocate(a, b)
+    allocate ( darray(a, b) ) 
+
----------------
whitespace at end.


================
Comment at: flang/test/Semantics/omp-allocate-directive.f90:16
+
+!$omp allocate(a, b) allocator(omp_default_mem_alloc)
+    allocate ( darray(a, b) ) 
----------------
Is a test allowed with multiple clauses or will it be handled in semantic phase?
```
!$omp allocate(x, y) allocator(omp_default_mem_alloc) allocator(omp_default_mem_alloc)
```


================
Comment at: flang/test/Semantics/omp-allocate-directive.f90:17
+!$omp allocate(a, b) allocator(omp_default_mem_alloc)
+    allocate ( darray(a, b) ) 
+
----------------
whitespace at end.


================
Comment at: flang/test/Semantics/omp-allocate-directive.f90:23
+!$omp allocate(n)
+    allocate ( darray(z, t) ) 
+
----------------
whitespace at end.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89562/new/

https://reviews.llvm.org/D89562