Page MenuHomePhabricator

[MLIR][OpenMP] Distribute Construct Operation
Needs RevisionPublic

Authored by abidmalikwaterloo on Jul 7 2021, 12:35 PM.

Details

Summary

OpenMP distribute construct for MLIR operation and added a test case.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
abidmalikwaterloo marked 2 inline comments as done.May 18 2022, 11:28 AM
abidmalikwaterloo added inline comments.
mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
494–496

Ok

534

Can you say more about the semantic checks you have in mind?

shraiysh added inline comments.May 18 2022, 12:06 PM
clang/lib/Testing/CMakeLists.txt
29 ↗(On Diff #428955)

Hmm, these are in llvm-project/main right now but this means that the patch has not been rebased properly. These changes should not be a part of this patch :/ Can you try rebasing again? Otherwise this will cause issues while/after landing this patch.

mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
506

They are being removed because we do not have a clean way of handling these clauses in MLIR for any generic frontend.

520

If and when it changes in the standard, at that time we can change the parsing/printing accordingly. Till then such a function seems unnecessary and a possible source of errors because it accepts invalid OpenMP code.

534

The following restriction from the standard can be added to the verifier/Operation definition -

The region corresponding to the distribute construct must be strictly nested inside a teams region.

The other restrictions - I am okay with not adding them because I don't know how they would be added. Needless to say if we figure out how to add them, then we should do it.

abidmalikwaterloo marked an inline comment as done.May 24 2022, 9:04 AM
abidmalikwaterloo added inline comments.
clang/lib/Testing/CMakeLists.txt
29 ↗(On Diff #428955)

git rebase origin arcpatch-D105584 gives " The current branch is up to date". It means the patch is up to date. Should I remove them manually?

mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
520

Should it be then? dist_schedule ( (static ) (: $schedule_chunk^)? )
$static_dist_schedule seems redundant???

534

At this stage, we can add:

LogicalResult DistributeOp::verify(){

return success();

}

We can add the check later!

mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
300

I do not think we need this if we treat the schedule as a variable?

shraiysh added inline comments.May 26 2022, 7:28 PM
clang/lib/Testing/CMakeLists.txt
29 ↗(On Diff #428955)

I'm not sure what the issue is, but this change should not be reflected here if the patch is properly rebased with main.

mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
520

AFAIK, the keyword static is optional. Without the presence of $static_dist_schedule, how are you going to store this flag?

534

Why not add the check now itself. Can it not be implemented? It's a small check :/

mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
300

Yup. We probably don't need these functions.

613

Change this comment to Loop control.

abidmalikwaterloo marked an inline comment as done.Jun 27 2022, 12:46 PM
abidmalikwaterloo added inline comments.
clang/lib/Testing/CMakeLists.txt
29 ↗(On Diff #428955)

I will remove them manually. I think rebase is correct.

mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
534

done

mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
300

done

abidmalikwaterloo added a comment.EditedJun 28 2022, 7:33 PM
// 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?

// 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) ({?

// 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.

abidmalikwaterloo added a comment.EditedJun 29 2022, 10:52 AM
// 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.

It is working. I will up load the patch. Thanks

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

abidmalikwaterloo marked 11 inline comments as done.

Updated DistributeOp::verify as per reviewer's comments

abidmalikwaterloo marked an inline comment as done.Jul 5 2022, 8:55 AM

update the changes

clementval added inline comments.Jul 5 2022, 9:03 AM
clang/lib/Testing/CMakeLists.txt
30 ↗(On Diff #442321)

You still have these changes that are unrelated.

Removed unrelated changes in CMakefile.txt

clementval added inline comments.Jul 11 2022, 1:26 PM
mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
455

Are you still working on this? Otherwise please close it.

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.

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

I still see some review comments not done.

abidmalikwaterloo marked 2 inline comments as done.Nov 4 2022, 11:48 AM
abidmalikwaterloo added inline comments.
mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
455

What's the problem here? It seems fine to me.

clementval added inline comments.Nov 4 2022, 12:55 PM
mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
455

syntax doesn't match the existing code. Change is highlighted.

abidmalikwaterloo marked 2 inline comments as done.

corrected the syntex of the code

abidmalikwaterloo marked 2 inline comments as done.Nov 7 2022, 9:31 AM
abidmalikwaterloo marked an inline comment as done.
kiranchandramohan requested changes to this revision.Nov 9 2022, 6:18 AM

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

mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
496

Do you mean static_dist_schedule here?

501–502

We can remove collapse for now.

519

In https://reviews.llvm.org/D128338 we removed collapse from all constructs. Currently collapse is modelled by having multiple entries in the lowerBound, upperBound and step.

mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
285
294

Nit: Check that their sizes are equal as well. And also if step is present then its size matches lowerbound/upperbound.

mlir/test/Dialect/OpenMP/ops.mlir
136

Add a pretty-print test as well.

This revision now requires changes to proceed.Nov 9 2022, 6:18 AM

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

With which branch should I rebase? I have taken care rest of the comments. Thanks!

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

With which branch should I rebase? I have taken care rest of the comments. Thanks!

Rebase to the main branch.

abidmalikwaterloo marked 3 inline comments as done.

Update the patch based on the feedback and comments.

kiranchandramohan requested changes to this revision.Dec 8 2022, 5:47 AM

@abidmalikwaterloo It seems you missed a few of the previous comments. Please fix these so that we can approve.

mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
455

Nit: Please fix this (adding a space before "DistributeOp").

mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
285

Nit: Please make this change.

294

Nit: Please make this change as well.

mlir/test/Dialect/OpenMP/ops.mlir
136

Nit: please add a pretty-print test.

This revision now requires changes to proceed.Dec 8 2022, 5:47 AM
abidmalikwaterloo marked 5 inline comments as done.Dec 21 2022, 8:37 AM
abidmalikwaterloo added inline comments.
mlir/test/Dialect/OpenMP/ops.mlir
136

Just need clarification. Do you mean something similar to the following:

// CHECK-LABEL: omp_wsloop_pretty
func.func @omp_wsloop_pretty(%lb : index, %ub : index, %step : index, %data_var : memref<i32>, %linear_var : i32, %chunk_var : i32, %chunk_var2 : i16) -> () {

  // CHECK: omp.wsloop ordered(2)
  // CHECK-SAME: for (%{{.*}}) : index = (%{{.*}}) to (%{{.*}}) step (%{{.*}})
  omp.wsloop ordered(2)
  for (%iv) : index = (%lb) to (%ub) step (%step) {
    omp.yield
  }

  // CHECK: omp.wsloop linear(%{{.*}} = %{{.*}} : memref<i32>) schedule(static)
  // CHECK-SAME: for (%{{.*}}) : index = (%{{.*}}) to (%{{.*}}) step (%{{.*}})
  omp.wsloop schedule(static) linear(%data_var = %linear_var : memref<i32>)
  for (%iv) : index = (%lb) to (%ub) step (%step) {
    omp.yield
  }

  // CHECK: omp.wsloop linear(%{{.*}} = %{{.*}} : memref<i32>) schedule(static = %{{.*}} : i32) ordered(2)
  // CHECK-SAME: for (%{{.*}}) : index = (%{{.*}}) to (%{{.*}}) step (%{{.*}})
  omp.wsloop ordered(2) linear(%data_var = %linear_var : memref<i32>) schedule(static = %chunk_var : i32)
  for (%iv) : index = (%lb) to (%ub) step (%step) {
    omp.yield
  }

  // CHECK: omp.wsloop linear(%{{.*}} = %{{.*}} : memref<i32>) schedule(dynamic = %{{.*}} : i32, nonmonotonic) ordered(2)
  // CHECK-SAME: for (%{{.*}}) : index = (%{{.*}}) to (%{{.*}}) step (%{{.*}})
  omp.wsloop ordered(2) linear(%data_var = %linear_var : memref<i32>) schedule(dynamic = %chunk_var : i32, nonmonotonic)
  for (%iv) : index = (%lb) to (%ub) step (%step)  {
    omp.yield
  }

  // CHECK: omp.wsloop linear(%{{.*}} = %{{.*}} : memref<i32>) schedule(dynamic = %{{.*}} : i16, monotonic) ordered(2)
  // CHECK-SAME: for (%{{.*}}) : index = (%{{.*}}) to (%{{.*}}) step (%{{.*}})
  omp.wsloop ordered(2) linear(%data_var = %linear_var : memref<i32>) schedule(dynamic = %chunk_var2 : i16, monotonic)
  for (%iv) : index = (%lb) to (%ub) step (%step) {
    omp.yield
  }

  // CHECK: omp.wsloop for (%{{.*}}) : index = (%{{.*}}) to (%{{.*}}) step (%{{.*}})
  omp.wsloop for (%iv) : index = (%lb) to (%ub) step (%step) {
    omp.yield
  }

  // CHECK: omp.wsloop for (%{{.*}}) : index = (%{{.*}}) to (%{{.*}}) inclusive step (%{{.*}})
  omp.wsloop for (%iv) : index = (%lb) to (%ub) inclusive step (%step) {
    omp.yield
  }

  // CHECK: omp.wsloop nowait
  // CHECK-SAME: for (%{{.*}}) : index = (%{{.*}}) to (%{{.*}}) step (%{{.*}})
  omp.wsloop nowait
  for (%iv) : index = (%lb) to (%ub) step (%step) {
    omp.yield
  }

  // CHECK: omp.wsloop nowait order(concurrent)
  // CHECK-SAME: for (%{{.*}}) : index = (%{{.*}}) to (%{{.*}}) step (%{{.*}})
  omp.wsloop order(concurrent) nowait
  for (%iv) : index = (%lb) to (%ub) step (%step) {
    omp.yield
  }

  return
}
mlir/test/Dialect/OpenMP/ops.mlir
136

Yes.

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

abidmalikwaterloo marked 2 inline comments as done.Thu, Jan 5, 7:16 PM

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

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

Ok..make sense

kiranchandramohan requested changes to this revision.Thu, Jan 26, 2:29 AM

Use omp.canonical_loop once it is in.

This revision now requires changes to proceed.Thu, Jan 26, 2:29 AM