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

261

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–73

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

660

Nit: Remove commented code.

661

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

664

684

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

9 ↗

(On Diff #507838)

Might be good to specify different sizes for each loop.

22–25 ↗

(On Diff #507838)

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

660

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

680

Same here.

[abidmalikwaterloo]

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

660

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

660

So you should use evaluate::ToInt64 directly.

[kiranchandramohan]

@do Will you be taking over this patch?

flang/lib/Parser/openmp-parsers.cpp

TYPE_PARSER(
     "SCHEDULE" >> construct<OmpClause>(construct<OmpClause::Schedule>(
                       parenthesized(Parser<OmpScheduleClause>{}))) ||
     "SEQ_CST" >> construct<OmpClause>(construct<OmpClause::SeqCst>()) ||
     "SHARED" >> construct<OmpClause>(construct<OmpClause::Shared>(
                     parenthesized(Parser<OmpObjectList>{}))) ||
     "SIMD"_id >> construct<OmpClause>(construct<OmpClause::Simd>()) ||
     "SIMDLEN" >> construct<OmpClause>(construct<OmpClause::Simdlen>(
                      parenthesized(scalarIntConstantExpr))) ||
-    "SIZES" >> construct<OmpClause>(construct<OmpClause::Sizes>(
-                   parenthesized(nonemptyList(scalarIntExpr)))) ||
+    "SIZES" >> construct<OmpClause>(
+                   construct<OmpClause::Sizes>(parenthesized(scalarIntExpr))) ||
     "THREADS" >> construct<OmpClause>(construct<OmpClause::Threads>()) ||
     "THREAD_LIMIT" >> construct<OmpClause>(construct<OmpClause::ThreadLimit>(
                           parenthesized(scalarIntExpr))) ||
     "TO" >> construct<OmpClause>(construct<OmpClause::To>(
                 parenthesized(Parser<OmpObjectList>{}))) ||
     "USE_DEVICE_PTR" >> construct<OmpClause>(construct<OmpClause::UseDevicePtr>(
                             parenthesized(nonemptyList(name)))) ||
     "UNIFIED_ADDRESS" >>

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 CheckMultListItems();
   bool HasInvalidWorksharingNesting(
       const parser::CharBlock &, const OmpDirectiveSet &);
   bool IsCloselyNestedRegion(const OmpDirectiveSet &set);
   void HasInvalidTeamsNesting(
       const llvm::omp::Directive &dir, const parser::CharBlock &source);
   void HasInvalidDistributeNesting(const parser::OpenMPLoopConstruct &x);
+  bool HasInvalidTileNesting(
+      const llvm::omp::Directive &dir, const parser::CharBlock &source);
+  bool HasInvalidUnrollNesting(
+      const llvm::omp::Directive &dir, const parser::CharBlock &source);
   // specific clause related
   bool ScheduleModifierHasType(const parser::OmpScheduleClause &,
       const parser::OmpScheduleModifierType::ModType &);
   void CheckAllowedMapTypes(const parser::OmpMapType::Type &,
       const std::list<parser::OmpMapType::Type> &);
   llvm::StringRef getClauseName(llvm::omp::Clause clause) override;
   llvm::StringRef getDirectiveName(llvm::omp::Directive directive) override;
 
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 CheckTheRectangleTileNesting(const parser::OpenMPBlockConstruct &x);
+  // void CheckThePerfectNesting(const parser::OpenMPBlockConstruct &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);
+  }
+  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)}) {
+      const auto u = GetIntValue(sizeClause->v);
+      if (*u < 0) {
+        context_.Say(clause.source,
+            "The parameter value of the Size 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 auto 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/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::j 
+  !ERROR: The value of the parameter in the size clause must 
+  !be greater than 0  
+        !$omp tile sizes(-1) 
+         do j = 1,10 
+         print *, "hello" 
+         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 Cconstruct
+
+program omp_doUnroll 
+  integer::j 
+  !ERROR: The value of the parameter in the partial clause must be greater than 0 
+  !$omp unroll partial(-1) 
+  do j = 1,10 
+    print *,"hello" 
+  end do 
+  !$omp end unroll 
+end program omp_doUnroll

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

def OMPC_Default : Clause<"default"> {
   let flangClass = "OmpDefaultClause";
 }
 def OMPC_Private : Clause<"private"> {
   let clangClass = "OMPPrivateClause";
   let flangClass = "OmpObjectList";
 }
 def OMPC_Sizes: Clause<"sizes"> { 
   let clangClass = "OMPSizesClause"; 
   let flangClass = "ScalarIntExpr";
-  let isValueList = true;
   }
 def OMPC_Full: Clause<"full"> { 
   let clangClass = "OMPFullClause"; 
 }
 def OMPC_Partial: Clause<"partial"> { 
   let clangClass = "OMPPartialClause";
   let flangClass = "ScalarIntConstantExpr"; 
   let isValueOptional = true;
  }