Flang semantic check support for tile and unroll OpenMP Directive.

[abidmalikwaterloo]

Summary

Added semantic check support for tile and unroll OpenMP Directive.
OpenMP 5.1

[clementval]

Looks like smth is missing from your patch. There is also clang-format errors.

[abidmalikwaterloo]

In D142796#4088750, @clementval wrote:

Looks like smth is missing from your patch. There is also clang-format errors.

I just uploaded the header file to take feedback.

[clementval]

In D142796#4092378, @abidmalikwaterloo wrote:

In D142796#4088750, @clementval wrote:

Looks like smth is missing from your patch. There is also clang-format errors.

I just uploaded the header file to take feedback.

Well there is not much to comment on.

[abidmalikwaterloo]

In D142796#4092692, @clementval wrote:

In D142796#4092378, @abidmalikwaterloo wrote:

In D142796#4088750, @clementval wrote:

Looks like smth is missing from your patch. There is also clang-format errors.

I just uploaded the header file to take feedback.

Well there is not much to comment on.

I will upload the implementation and test cases as well

[abidmalikwaterloo]

Update the patch with basic support for semantic checks of
OpenMP Tile and Unroll Constructs
OpenMP 5.1

[abidmalikwaterloo]

Corrected the format of tests

[abidmalikwaterloo]

Format correction for omp-tile-size.f90

[abidmalikwaterloo]

Remove redundent code

[abidmalikwaterloo]

Ping!

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

257

Are there any more checks needed at this stage? Besides checking integer expression, the length of the list should be equal to the depth of the nested loops

flang/test/Semantics/OpenMP/omp-do-tile.f90

4

The build is failing on the tests. Is this the right way to write the semantic tests for Flang?

llvm/include/llvm/Frontend/OpenMP/OMP.td

72 ↗

(On Diff #501738)

We need a list of integers to take care of the nested loops

!$omp tile sizes(size-list) 
   loop-nest 
[!$omp end tile]

[luporl]

flang/test/Semantics/OpenMP/omp-do-tile.f90

3

Nit: it's better to put the !2.11.9.1 tile Cconstruct part on a new line.

4

There are extra spaces in your RUN line. This should work:
! RUN: %python %S/../test_errors.py %s %flang -fopenmp

7–8

test_errors.py will fail to match the error string here. It must be in a single line.

[abidmalikwaterloo]

Made changes to take semantic checks for OPENMP TILE--> SIZE clause so it
can take list of integers

[abidmalikwaterloo]

Made corrcection in Formating

[abidmalikwaterloo]

Added test for tile size clause

[abidmalikwaterloo]

Make correction to the format of OMP.td

[clementval]

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

424–425

I don't think the directive can be tile and unroll at the same time.

630

Remove

[clementval]

flang/test/Semantics/OpenMP/omp-do-tile.f90

9

Can you add test with non-constant integer. The standard does precise that

size-list is a list s1,…,sn of positive integer expressions.

[abidmalikwaterloo]

flang/test/Semantics/OpenMP/omp-do-tile.f90

9

Do you mean initialing a variable and then using it in the sizes clause: e.g. sizes (2, x, 2)?

[abidmalikwaterloo]

Made changes based on the comments. No it does not check
for both tile and unroll constructs.

[abidmalikwaterloo]

ping

[kiranchandramohan]

Thanks for this patch. Have some request for changes.

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

659

Nit: Remove commented code.

660

Nit: Do not use auto, specify the type here.

663

683

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

63–73

Are these changes required?

72

Why is this change required?

flang/test/Parser/omp-tile-multisize.f90

10

Might be good to specify different sizes for each loop.

23–26

You have to check that the size for all loops. Only one is checked here.

flang/test/Semantics/OpenMP/omp-do-tile.f90

8

9

Yeah, an expression like 2 + 2.
Also a variable that is defined as constant.

integer, parameter :: x = -3
!$omp unroll partial(x)
...

flang/test/Semantics/OpenMP/omp-do-unroll.f90

4

[abidmalikwaterloo]

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

63–73

My understanding is that the tile/unroll constructs can be called from the most loop-related directives. That's why I make both of them part of the set.

72

Again, my understanding is there will set of constructs that will be called from the tileset.

[abidmalikwaterloo]

Made changes in check-omp-structure.cpp as per reviewer's comments. Added two new tests.

[clementval]

flang/test/Semantics/OpenMP/omp-do-tile.f90

9

Do you mean initialing a variable and then using it in the sizes clause: e.g. sizes (2, x, 2)?

yes

[abidmalikwaterloo]

flang/test/Semantics/OpenMP/omp-do-tile.f90

9

Already included for tiles. Will include a test for unrolling as well

[abidmalikwaterloo]

Added a new test for unrolling construct with add expression

[tschuett]

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

659

Do you need to check whether u is std::nullopt first before you do *u?

679

Same here.

[abidmalikwaterloo]

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

659

If it is std::nullopt, I do not think it is a semantic error. It will be a bug then. You initiate it when you see something in the clause. It can't be null. This is my understanding.

[tschuett]

You always have to check optionals, before you can access its content.

[clementval]

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

659

So you should use evaluate::ToInt64 directly.

[kiranchandramohan]

@do Will you be taking over this patch?

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

static OmpDirectiveSet doSimdSet{Directive::OMPD_distribute_parallel_do_simd,
     Directive::OMPD_target_parallel_do_simd,
     Directive::OMPD_target_teams_distribute_parallel_do_simd,
     Directive::OMPD_teams_distribute_parallel_do_simd};
 static OmpDirectiveSet workShareSet{
     OmpDirectiveSet{Directive::OMPD_workshare,
         Directive::OMPD_parallel_workshare, Directive::OMPD_parallel_sections,
         Directive::OMPD_sections, Directive::OMPD_single} |
     doSet};
-static OmpDirectiveSet taskloopSet{
-    Directive::OMPD_taskloop, Directive::OMPD_taskloop_simd};
+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,
-    Directive::OMPD_target_teams_distribute_simd};
+    Directive::OMPD_target_teams_distribute_simd, Directive::OMPD_tile,
+    OMPD_unroll};
+static OmpDirectiveSet tileSet{Directive::OMPD_tile};
 static OmpDirectiveSet simdSet{Directive::OMPD_distribute_parallel_do_simd,
     Directive::OMPD_distribute_simd, Directive::OMPD_parallel_do_simd,
     Directive::OMPD_do_simd, Directive::OMPD_simd,
     Directive::OMPD_target_parallel_do_simd,
     Directive::OMPD_target_teams_distribute_parallel_do_simd,
     Directive::OMPD_target_teams_distribute_simd, Directive::OMPD_target_simd,
     Directive::OMPD_taskloop_simd,
     Directive::OMPD_teams_distribute_parallel_do_simd,
     Directive::OMPD_teams_distribute_simd};
 static OmpDirectiveSet teamSet{Directive::OMPD_teams,
     Directive::OMPD_teams_distribute,
     Directive::OMPD_teams_distribute_parallel_do,
     Directive::OMPD_teams_distribute_parallel_do_simd,
     Directive::OMPD_teams_distribute_parallel_for,
     Directive::OMPD_teams_distribute_parallel_for_simd,
     Directive::OMPD_teams_distribute_simd};
 static OmpDirectiveSet taskGeneratingSet{
     OmpDirectiveSet{Directive::OMPD_task} | taskloopSet};
+static OmpDirectiveSet unrollSet{Directive::OMPD_unroll};
 static OmpDirectiveSet nestedOrderedErrSet{Directive::OMPD_critical,
     Directive::OMPD_ordered, Directive::OMPD_atomic, Directive::OMPD_task,
     Directive::OMPD_taskloop};
 static OmpDirectiveSet nestedWorkshareErrSet{
     OmpDirectiveSet{Directive::OMPD_task, Directive::OMPD_taskloop,
         Directive::OMPD_critical, Directive::OMPD_ordered,
         Directive::OMPD_atomic, Directive::OMPD_master} |
     workShareSet};
private:
   void CheckCancellationNest(
       const parser::CharBlock &source, const parser::OmpCancelType::Type &type);
   std::int64_t GetOrdCollapseLevel(const parser::OpenMPLoopConstruct &x);
   bool CheckReductionOperators(const parser::OmpClause::Reduction &);
   bool CheckIntrinsicOperator(
       const parser::DefinedOperator::IntrinsicOperator &);
   void CheckReductionTypeList(const parser::OmpClause::Reduction &);
   void CheckMasterNesting(const parser::OpenMPBlockConstruct &x);
+  void CheckTileConstruct(const parser::OpenMPLoopConstruct &x);
+  void CheckUnrollConstruct(const parser::OpenMPLoopConstruct &x);
   void ChecksOnOrderedAsBlock();
   void CheckBarrierNesting(const parser::OpenMPSimpleStandaloneConstruct &x);
   void ChecksOnOrderedAsStandalone();
   void CheckOrderedDependClause(std::optional<std::int64_t> orderedValue);
   void CheckReductionArraySection(const parser::OmpObjectList &ompObjectList);
   void CheckIntentInPointerAndDefinable(
       const parser::OmpObjectList &, const llvm::omp::Clause);
   void CheckArraySection(const parser::ArrayElement &arrayElement,

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

if (GetDirectiveNest(TargetNest) > 0) {
       CheckTargetNest(x);
     }
   }
 }
 
 void OmpStructureChecker::Enter(const parser::OpenMPLoopConstruct &x) {
   const auto &beginLoopDir{std::get<parser::OmpBeginLoopDirective>(x.t)};
   const auto &beginDir{std::get<parser::OmpLoopDirective>(beginLoopDir.t)};
-
   // check matching, End directive is optional
   if (const auto &endLoopDir{
           std::get<std::optional<parser::OmpEndLoopDirective>>(x.t)}) {
     const auto &endDir{
         std::get<parser::OmpLoopDirective>(endLoopDir.value().t)};
 
     CheckMatching<parser::OmpLoopDirective>(beginDir, endDir);
   }
void OmpStructureChecker::Enter(const parser::OpenMPLoopConstruct &x) {
   CheckDoWhile(x);
   CheckLoopItrVariableIsInt(x);
   CheckCycleConstraints(x);
   HasInvalidDistributeNesting(x);
   if (CurrentDirectiveIsNested() &&
       llvm::omp::teamSet.test(GetContextParent().directive)) {
     HasInvalidTeamsNesting(beginDir.v, beginDir.source);
   }
+  if (beginDir.v == llvm::omp::Directive::OMPD_tile) {
+    CheckTileConstruct(x);
+  }
+  else if (beginDir.v == llvm::omp::Directive::OMPD_unroll) {
+    CheckUnrollConstruct(x);
+  }
+
   if ((beginDir.v == llvm::omp::Directive::OMPD_distribute_parallel_do_simd) ||
       (beginDir.v == llvm::omp::Directive::OMPD_distribute_simd)) {
     CheckDistLinear(x);
   }
 }
 const parser::Name OmpStructureChecker::GetLoopIndex(
     const parser::DoConstruct *x) {
   using Bounds = parser::LoopControl::Bounds;
if (const auto *collapseClause{
             std::get_if<parser::OmpClause::Collapse>(&clause.u)}) {
       if (const auto v{GetIntValue(collapseClause->v)}) {
         collapseLevel = *v;
       }
     }
     if (const auto *orderedClause{
             std::get_if<parser::OmpClause::Ordered>(&clause.u)}) {
       if (const auto v{GetIntValue(orderedClause->v)}) {
         orderedLevel = *v;
       }
     }
   }
   if (orderedLevel >= collapseLevel) {
     orderedCollapseLevel = orderedLevel;
   } else {
     orderedCollapseLevel = collapseLevel;
   }
   return orderedCollapseLevel;
 }
 
 void OmpStructureChecker::CheckCycleConstraints(
     const parser::OpenMPLoopConstruct &x) {
   std::int64_t ordCollapseLevel{GetOrdCollapseLevel(x)};
   OmpCycleChecker ompCycleChecker{context_, ordCollapseLevel};
   parser::Walk(x, ompCycleChecker);
 }
 
+void OmpStructureChecker::CheckTileConstruct(
+    const parser::OpenMPLoopConstruct &x) {
+
+  const auto &beginLoopDir{std::get<parser::OmpBeginLoopDirective>(x.t)};
+  const auto &clauseList{std::get<parser::OmpClauseList>(beginLoopDir.t)};
+
+  for (const auto &clause : clauseList.v) {
+    if (const auto *sizeClause{
+            std::get_if<parser::OmpClause::Sizes>(&clause.u)}) {
+      for (const auto &s : sizeClause->v) {
+         const std::optional<std::int64_t> u = GetIntValue(s);
+        if (*u < 0) {
+          context_.Say(clause.source,
+              "The parameter value of the SIZES clause must be greater than "
+              "zero "_err_en_US);
+        }
+      }
+    }
+  }
+}
+
+void OmpStructureChecker::CheckUnrollConstruct(
+    const parser::OpenMPLoopConstruct &x) {
+
+  const auto &beginLoopDir{std::get<parser::OmpBeginLoopDirective>(x.t)};
+  const auto &clauseList{std::get<parser::OmpClauseList>(beginLoopDir.t)};
+
+  for (const auto &clause : clauseList.v) {
+    if (const auto *sizeClause{
+            std::get_if<parser::OmpClause::Partial>(&clause.u)}) {
+      const std::optional<std::int64_t> u = GetIntValue(sizeClause->v);
+      if (*u < 1) {
+        context_.Say(clause.source,
+            "The parameter value of the PARTIAL clause must be greater than "
+            "one "_err_en_US);
+      }
+    }
+  }
+}
+
 void OmpStructureChecker::CheckDistLinear(
     const parser::OpenMPLoopConstruct &x) {
 
   const auto &beginLoopDir{std::get<parser::OmpBeginLoopDirective>(x.t)};
   const auto &clauses{std::get<parser::OmpClauseList>(beginLoopDir.t)};
 
   semantics::UnorderedSymbolSet indexVars;
 

flang/test/Parser/omp-tile-multisize.f90

+! RUN: %flang_fc1 -fdebug-unparse -fopenmp %s | FileCheck --ignore-case %s
+! RUN: %flang_fc1 -fdebug-dump-parse-tree -fopenmp %s | FileCheck --check-prefix="PARSE-TREE" %s   
+
+subroutine openmp_tiles(x, y, z)
+
+integer, intent(inout)::x, y, z
+
+!CHECK : !$omp tile sizes 
+!$omp tile sizes(2, 3, 4) 
+!CHECK : do 
+do x = 1, 100 
+do y = 1, 100 
+do z = 1, 100 
+   call F1() 
+!CHECK: end do 
+end do 
+end do 
+end do 
+!CHECK : !$omp end tile 
+!$omp end tile
+
+!PARSE-TREE: OpenMPConstruct->OpenMPLoopConstruct
+!PARSE-TREE: OmpBeginLoopDirective 
+!PARSE-TREE: OmpLoopDirective->llvm::omp::Directive = tile
+!PARSE-TREE: OmpClauseList->OmpClause->Sizes->Scalar->Integer->Expr->LiteralConstant -> IntLiteralConstant = '2'
+!PARSE-TREE: Scalar -> Integer -> Expr -> LiteralConstant -> IntLiteralConstant = '3'
+!PARSE-TREE: Scalar -> Integer -> Expr -> LiteralConstant -> IntLiteralConstant = '4'
+
+END subroutine openmp_tiles
+    
+ 

flang/test/Parser/omp-tile-size.f90

 ! RUN: %flang_fc1 -fdebug-unparse -fopenmp %s | FileCheck --ignore-case %s
 ! RUN: %flang_fc1 -fdebug-dump-parse-tree -fopenmp %s | FileCheck --check-prefix="PARSE-TREE" %s
 
 subroutine openmp_tiles(x)
 
-  integer, intent(inout)::x
+  integer, intent(inout)::x 
 
 !CHECK: !$omp tile sizes
 !$omp  tile sizes(2)
 !CHECK: do
   do x = 1, 100
     call F1()
 !CHECK: end do
   end do
 !CHECK: !$omp end tile
 !$omp end tile
 
 
 !PARSE-TREE: OpenMPConstruct -> OpenMPLoopConstruct
 !PARSE-TREE: OmpBeginLoopDirective
 !PARSE-TREE: OmpLoopDirective -> llvm::omp::Directive = tile
-!PARSE-TREE: OmpClauseList -> OmpClause -> Sizes -> Scalar -> Integer -> Expr = '2_4'
+!PARSE-TREE: OmpClauseList -> OmpClause -> Sizes -> Scalar -> Integer -> Expr-> LiteralConstant -> IntLiteralConstant = '2'
 END subroutine openmp_tiles

flang/test/Parser/omp-tile-size2.f90

 ! RUN: %flang_fc1 -fdebug-unparse -fopenmp %s | FileCheck --ignore-case %s
 ! RUN: %flang_fc1 -fdebug-dump-parse-tree -fopenmp %s | FileCheck --check-prefix="PARSE-TREE" %s
 
 subroutine openmp_tiles(x)
 
-  integer, intent(inout)::x
-
+  integer, intent(inout)::x 
+  integer, parameter :: k = 3 
 !CHECK: !$omp tile sizes
-!$omp  tile sizes(2)
+!$omp  tile sizes(k)
 !CHECK: do
   do x = 1, 100
     call F1()
 !CHECK: end do
   end do
 !CHECK: !$omp end tile
 !$omp end tile
 
-
+!PARSE-TREE: Initialization -> Constant -> Expr -> LiteralConstant -> IntLiteralConstant = '3'
 !PARSE-TREE: OpenMPConstruct -> OpenMPLoopConstruct
 !PARSE-TREE: OmpBeginLoopDirective
 !PARSE-TREE: OmpLoopDirective -> llvm::omp::Directive = tile
-!PARSE-TREE: OmpClauseList -> OmpClause -> Sizes -> Scalar -> Integer -> Expr = '2_4'
+!PARSE-TREE: OmpClauseList -> OmpClause -> Sizes -> Scalar -> Integer -> Expr-> Designator -> DataRef -> Name = 'k'
 END subroutine openmp_tiles

flang/test/Parser/omp-tile-size3.f90

 ! RUN: %flang_fc1 -fdebug-unparse -fopenmp %s | FileCheck --ignore-case %s
 ! RUN: %flang_fc1 -fdebug-dump-parse-tree -fopenmp %s | FileCheck --check-prefix="PARSE-TREE" %s
 
 subroutine openmp_tiles(x)
 
-  integer, intent(inout)::x
+  integer, intent(inout)::x 
 
 !CHECK: !$omp tile sizes
-!$omp  tile sizes(2)
+!$omp  tile sizes(2+1)
 !CHECK: do
   do x = 1, 100
     call F1()
 !CHECK: end do
   end do
 !CHECK: !$omp end tile
 !$omp end tile
 
 
 !PARSE-TREE: OpenMPConstruct -> OpenMPLoopConstruct
 !PARSE-TREE: OmpBeginLoopDirective
 !PARSE-TREE: OmpLoopDirective -> llvm::omp::Directive = tile
-!PARSE-TREE: OmpClauseList -> OmpClause -> Sizes -> Scalar -> Integer -> Expr = '2_4'
+!PARSE-TREE: OmpClauseList -> OmpClause -> Sizes -> Scalar -> Integer -> Expr-> Add
+!PARSE-TREE: Expr -> LiteralConstant -> IntLiteralConstant = '2'
+!PARSE-TREE: Expr -> LiteralConstant -> IntLiteralConstant = '1'
 END subroutine openmp_tiles

flang/test/Parser/omp-unroll_2.f90

+! RUN: %flang_fc1 -fdebug-unparse-no-sema -fopenmp %s | FileCheck --ignore-case %s
+! RUN: %flang_fc1 -fdebug-dump-parse-tree -fopenmp %s | FileCheck --check-prefix="PARSE-TREE" %s
+
+subroutine openmp_parse_unroll(x)
+
+  integer, intent(inout)::x
+
+!CHECK: !$omp unroll partial
+!$omp  unroll partial(2+2)
+!CHECK: do
+  do x = 1, 100
+    call F1()
+!CHECK: end do
+  end do
+!CHECK: !$omp end unroll
+!$omp end unroll
+
+!PARSE-TREE: OpenMPConstruct -> OpenMPLoopConstruct
+!PARSE-TREE: OmpBeginLoopDirective
+!PARSE-TREE: OmpLoopDirective -> llvm::omp::Directive = unroll
+!PARSE-TREE: OmpClauseList -> OmpClause -> Partial -> Scalar -> Integer -> Constant -> Expr = 'Add'
+!PARSE-TREE: Expr -> LiteralConstant -> IntLiteralConstant = '2'
+!PARSE-TREE: Expr -> LiteralConstant -> IntLiteralConstant = '2'
+
+END subroutine openmp_parse_unroll

flang/test/Semantics/OpenMP/omp-do-tile.f90

+!RUN: % python %S/../test_errors.py %s %flang -fopenmp 
+!OpenMP Version 5.1 
+!2.11.9.1 tile Cconstruct
+
+program omp_dotile 
+  integer::i, j, k,z
+  !ERROR: The parameter value in the SIZES clause must be greater than one 
+  !$omp tile sizes(2, -2, 2) 
+  do i = 1,100
+   do j  = 1,100
+    do k =  1,100
+    print *,"hello" 
+    end do
+   end do   
+  end do 
+  !$omp end tile 
+end program omp_dotile

flang/test/Semantics/OpenMP/omp-do-unroll.f90

+!RUN : %python %S/../test_errors.py %s %flang -fopenmp 
+!OpenMP Version 5.1 
+!2.11.9.2 unroll Construct
+
+ program omp_doUnroll 
+  integer::j 
+  integer, parameter :: x = 3 
+  !ERROR: The parameter value in the PARTIAL clause must be greater than one 
+  !$omp unroll partial(x) 
+  do j = 1,10 
+    print *,"hello" 
+  end do 
+  !$omp end unroll 
+end program omp_doUnroll