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

Make correction to the format of OMP.td

Added test for tile size clause

Made corrcection in Formating

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

Ping!

Remove redundent code

Format correction for omp-tile-size.f90

Corrected the format of tests

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

Rebase the patch to origin/main with all updates. Made changes
to make it buildable using the new changes.
(WIP).

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

In D142796#4088750, @clementval wrote:

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

LGTM

In D105584#4081228, @kiranchandramohan wrote:

Since the plan is to switch to the Canonical Style operation, maybe this can wait till the omp.canonical_loop changes are in?

@jz10. Thanks. The clang has meta-directive support for static conditions. Did you implement any new meta-directive support for your test?

In D138928#4042097, @jz10 wrote:

In D138928#4040309, @abidmalikwaterloo wrote:

@jz10 did you create these tests stand-alone or tested them using a meta directive implementation?

we created them to test meta-directive implementation

@jz10 did you create these tests stand-alone or tested them using a metadirective implementation?

Added a pretty test for omp.distrubute in ops.mlir

ping!

Updated the test case omp-unroll.f90

replace scalarIntExpr with scalarIntConstantExpr.

Ping!

Ping!

Should I commit it to the main branch?

Updated the test and took care of the reviewer's comments

Clean the formatting comments. The patch is out of attention
for a while. Still need to work on the context selector part
as a next step.

The tests do not include any runtime conditions.

Update the patch based on the feedback and comments.

In D105584#3917238, @kiranchandramohan wrote:

Patch probably needs a rebase. A few more minor things to fix. Looks mostly ready.

Ahh! I thought I fixed the clang-format issues. will look into it.

Added support for clauses. Added a new test case.

Added support for the clauses . Added new tests

In D136359#3935973, @kiranchandramohan wrote:

There is still an issue with the clang-format of flang/lib/Parser/unparse.cpp that is causing the CI to fail.

I also completely missed that the support for sizes clause is missing. We have to add support for parsing this clause in openmp-parsers.cpp and specify the type in the OMP.td. See relevant changes below. Feel free to use the suggested change if it looks OK.

Please also add a test with the sizes clause.

diff --git a/flang/lib/Parser/openmp-parsers.cpp b/flang/lib/Parser/openmp-parsers.cpp
index b4d41c899e96..032e3c6cf8ca 100644
--- a/flang/lib/Parser/openmp-parsers.cpp
+++ b/flang/lib/Parser/openmp-parsers.cpp
@@ -263,6 +263,8 @@ TYPE_PARSER(
     "SIMD"_id >> construct<OmpClause>(construct<OmpClause::Simd>()) ||
     "SIMDLEN" >> construct<OmpClause>(construct<OmpClause::Simdlen>(
                      parenthesized(scalarIntConstantExpr))) ||
+    "SIZES" >> construct<OmpClause>(construct<OmpClause::Sizes>(
+                   parenthesized(nonemptyList(scalarIntExpr)))) ||
     "THREADS" >> construct<OmpClause>(construct<OmpClause::Threads>()) ||
     "THREAD_LIMIT" >> construct<OmpClause>(construct<OmpClause::ThreadLimit>(
                           parenthesized(scalarIntExpr))) ||
diff --git a/llvm/include/llvm/Frontend/OpenMP/OMP.td b/llvm/include/llvm/Frontend/OpenMP/OMP.td
index 5f1d335ef04f..cac10c4c690f 100644
--- a/llvm/include/llvm/Frontend/OpenMP/OMP.td
+++ b/llvm/include/llvm/Frontend/OpenMP/OMP.td
@@ -67,7 +67,11 @@ def OMPC_Private : Clause<"private"> {
   let clangClass = "OMPPrivateClause";
   let flangClass = "OmpObjectList";
 }
-def OMPC_Sizes: Clause<"sizes"> { let clangClass = "OMPSizesClause"; }
+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"; }
 def OMPC_FirstPrivate : Clause<"firstprivate"> {

Fixed the problem with clang-format

In D136359#3931827, @kiranchandramohan wrote:

The build is failing. See inline for issue.

    "TILE" >> pure(llvm::omp::Directive::OMPD_tile))
    ^
/var/lib/buildkite-agent/builds/llvm-project/flang/lib/Parser/type-parser-implementation.h:23:9: note: macro 'TYPE_PARSER' defined here
#define TYPE_PARSER(pexpr) \
        ^
/var/lib/buildkite-agent/builds/llvm-project/flang/lib/Parser/openmp-parsers.cpp:285:1: error: unknown type name 'TYPE_PARSER'
TYPE_PARSER(sourced(construct<OmpLoopDirective>(first(
^
/var/lib/buildkite-agent/builds/llvm-project/flang/lib/Parser/openmp-parsers.cpp:321:1: error: expected unqualified-id
TYPE_PARSER(sourced(construct<OmpBeginLoopDirective>(
^
/var/lib/buildkite-agent/builds/llvm-project/flang/lib/Parser/type-parser-implementation.h:24:3: note: expanded from macro 'TYPE_PARSER'
  template <> \
  ^

Also, please clang-format the following files,

flang/lib/Parser/openmp-parsers.cpp
flang/lib/Parser/unparse.cpp

Made changes to openmp-parsers.cpp as pinpointed by the reviewer.

Updated the patch based on the comments. Also added simple test case.

I have a question about the test: The following passed the test

! 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

corrected the syntex of the code

Ping!
Does this need any further change? Seems okay to me.

Seems that everything has been taken care of. It was accidentally out of my radar (do not know why). I will go through it once again and update it.

Model Tile Construct as OpenMPLoopConstruct.
The patch contains the basic support.

It looks that the tile construct can be modeled as OpenMPLoopConstruct. But, how it will be linked to OmpLoopDirective? I am getting the following error:

@kiranchandramohan : You mean we should have :

This patch has been abandoned
check the following
https://reviews.llvm.org/D136359

Yes. Something went wrong. No changes showed up. I will submit it again.

In case:

Updated the PrintStmt.cpp according to the comments

I will work on the patch in parts. I am planning to submit taking care of comments except for comments for SemaOpenMP and code generation.

In D105255#3723940, @clementval wrote:

In D105255#3723865, @abidmalikwaterloo wrote:

[MLIR][OpenMP] Added target data, exit data, and enter data operation definition for MLIR

Looks like it's not the complete revision. There are almost no diffs anymore.

[MLIR][OpenMP] Added target data, exit data, and enter data operation definition for MLIR

In D105255#3707533, @clementval wrote:

In D105255#3707495, @raghavendhra wrote:

In D105255#3707088, @abidmalikwaterloo wrote:

In D105255#3706602, @raghavendhra wrote:

@abidmalikwaterloo Can you please check your last patch pushed for review? https://buildkite.com/llvm-project/diff-checks/builds/117796 states it can not apply your patch something to do with the usage of diff --update <patch>? Can you please check?

@raghavendhra I tried to correct it but could not figure out the solution. Do you have any? I am not experienced with the phabricator framework. One solution is to submit another patch with all changes till today and abandon this one.

Sorry, I am new to phabricator as well. I use "arc diff" to upload my revisions for review after addressing review comments. If no one responds probably I am fine with new review and abandoning this one.

Did you try arc diff --update D105255 from your branch?

In D130851#3696913, @Meinersbur wrote:

In D130851#3695118, @abidmalikwaterloo wrote:

• The full clause does not have an operand, it is more similar to a flag like nowait.

nowait is unitAtt. full can be treated the same way. I am considering it as boolean with 0 as false and 1 as true <

full(0) or full(1) feels clunkier than a flag. $full_operand is also an Optional, does it mean that full() is valid as well? Any reason to not also make it a unitAtt?

I will make it UnitAtt.

Partial Clause has unroll factor which is an integer expression. There is no sentinal value, I think!

I think I oversaw that it is an Optional<>, hence None would be that sentinel.

In D105255#3706602, @raghavendhra wrote:

@abidmalikwaterloo Can you please check your last patch pushed for review? https://buildkite.com/llvm-project/diff-checks/builds/117796 states it can not apply your patch something to do with the usage of diff --update <patch>? Can you please check?

In D130851#3694338, @Meinersbur wrote:

I am not familiar with the MLIR tablegen format, but there might two issues:

• The full clause does not have an operand, it is more similar to a flag like nowait.

nowait is unitAtt. full can be treated the same way. I am considering it as boolean with 0 as false and 1 as true <

• The partial clauses operand is optional. $partial_operand is AnyInteger. Is there a sentinal value for $partial_operand if the clauses operand is omitted?

I just updated the patch. Something went wrong. I used arc diff --update <patch> for uploading. However, it seems it is not comparing the last submitted patch.

Added assembly format
changed the type of mape_type_modifier

@raghavendhra @kiranchandramohan After listening to your presentation today, I am inclined towards keeping it separate. There is a big assumption that OpenMP and OpenACC will move hand in hand which might be true now but in the future, it might not be true.

Ping!

In D105255#3643428, @clementval wrote:

Why not adding the assembly format directly for these operations like your patch D105584?

Update ops.mlir test cases. llvm-lit is working and all tests are passing

Updated and clean the code based on the comments from
the reviewer!

Removed unrelated changes in CMakefile.txt

update the changes

Updated DistributeOp::verify as per reviewer's comments

Update the patch for assembly format. Change the WSLoopControl
to LoopControl. Added DistributeOp to parent of Yield OP. Added
one test case

In D105584#3618040, @abidmalikwaterloo wrote:

In D105584#3617666, @clementval wrote:

In D105584#3617456, @abidmalikwaterloo wrote:

// CHECK-LABEL: omp_DistributeOp
func.func @omp_DistributeOp(%lb : index, %ub : index, %step : index, %data_var : memref<i32>, %chunk_var : i32) -> () {
   // CHECK: omp.DistributeOp collapse(2)
  "omp.DistributeOp" (%lb, %ub, %step) ({
    ^bb0(%iv: index):
     omp.yield
  }) {operand_segment_sizes = dense<[1,1,1,0,0]> : vector<5xi32>, collapse_val = 2} :
    (index, index, index) -> ()
 
 return
 }

Is this a valid test case for the operation?

Should it be "omp.distribute" (%lb, %ub, %step) ({?

Yes. It should be. The rest should be okay? But, the test is failing.

In D105584#3617666, @clementval wrote:

In D105584#3617456, @abidmalikwaterloo wrote:

// CHECK-LABEL: omp_DistributeOp
func.func @omp_DistributeOp(%lb : index, %ub : index, %step : index, %data_var : memref<i32>, %chunk_var : i32) -> () {
   // CHECK: omp.DistributeOp collapse(2)
  "omp.DistributeOp" (%lb, %ub, %step) ({
    ^bb0(%iv: index):
     omp.yield
  }) {operand_segment_sizes = dense<[1,1,1,0,0]> : vector<5xi32>, collapse_val = 2} :
    (index, index, index) -> ()
 
 return
 }

Is this a valid test case for the operation?

Should it be "omp.distribute" (%lb, %ub, %step) ({?