[PATCH] D142796: Flang semantic check support for tile and unroll OpenMP Directive.

[Kiran Chandramohan via Phabricator via llvm-commits]

kiranchandramohan requested changes to this revision.
kiranchandramohan added a comment.
This revision now requires changes to proceed.

Thanks for this patch. Have some request for changes.



================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:659
+      for (const auto &s : sizeClause->v) {
+        //const auto *type {
+        const auto u = GetIntValue(s);
----------------
Nit: Remove commented code.


================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:660
+        //const auto *type {
+        const auto u = GetIntValue(s);
+        if (*u < 0) {
----------------
Nit: Do not use auto, specify the type here.


================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:663
+          context_.Say(clause.source,
+              "The parameter value of the Size clause must be greater than "
+              "zero "_err_en_US);
----------------



================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:683
+        context_.Say(clause.source,
+            "The parameter value of the Partial clause must be greater than "
+            "one "_err_en_US);
----------------



================
Comment at: flang/lib/Semantics/check-omp-structure.h:63-71
+static OmpDirectiveSet taskloopSet{Directive::OMPD_taskloop,
+    Directive::OMPD_taskloop_simd, Directive::OMPD_tile,
+    Directive::OMPD_unroll};
 static OmpDirectiveSet targetSet{Directive::OMPD_target,
     Directive::OMPD_target_parallel, Directive::OMPD_target_parallel_do,
     Directive::OMPD_target_parallel_do_simd, Directive::OMPD_target_simd,
     Directive::OMPD_target_teams, Directive::OMPD_target_teams_distribute,
----------------
Are these changes required?


================
Comment at: flang/lib/Semantics/check-omp-structure.h:72
+    OMPD_unroll};
+static OmpDirectiveSet tileSet{Directive::OMPD_tile};
 static OmpDirectiveSet simdSet{Directive::OMPD_distribute_parallel_do_simd,
----------------
Why is this change required?


================
Comment at: flang/test/Parser/omp-tile-multisize.f90:9
+!CHECK : !$omp tile sizes 
+!$omp tile sizes(2, 2, 2) 
+!CHECK : do 
----------------
Might be good to specify different sizes for each loop.


================
Comment at: flang/test/Parser/omp-tile-multisize.f90:22-25
+!PARSE-TREE: OpenMPConstruct->OpenMPLoopConstruct
+!PARSE-TREE: OmpBeginLoopDirective 
+!PARSE-TREE: OmpLoopDirective->llvm::omp::Directive = tile
+!PARSE-TREE : OmpClauseList->OmpClause->Sizes->Scalar->Integer->Expr = '2_4' 
----------------
You have to check that the size for all loops. Only one is checked here.


================
Comment at: flang/test/Semantics/OpenMP/omp-do-tile.f90:7
+	integer::i, j, k,z
+	!ERROR: The parameter value in the Partial clause must be greater than one 
+	!$omp tile sizes(2, -2, 2) 
----------------



================
Comment at: flang/test/Semantics/OpenMP/omp-do-tile.f90:8
+	!ERROR: The parameter value in the Partial clause must be greater than one 
+	!$omp tile sizes(2, -2, 2) 
+	do i = 1,100
----------------
abidmalikwaterloo wrote:
> clementval wrote:
> > Can you add test with non-constant integer. The standard does precise that 
> > > size-list is a list s1,…,sn of positive integer expressions.
> Do you mean initialing a variable and then using it in the sizes clause: e.g. sizes (2, x, 2)?
Yeah, an expression like 2 + 2.
Also a variable that is defined as constant.
```
integer, parameter :: x = -3
!$omp unroll partial(x)
...
```


================
Comment at: flang/test/Semantics/OpenMP/omp-do-unroll.f90:3
+!OpenMP Version 5.1 
+!2.11.9.2 unroll Cconstruct
+
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142796