This is an archive of the discontinued LLVM Phabricator instance.

[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

abidmalikwaterloo requested review of this revision.Jul 7 2021, 12:35 PM
abidmalikwaterloo retitled this revision from Added definition for distribute construct for MLIR operation and added a test case. to [MLIR][OpenMP] Distribute Construct Operation.Jul 7 2021, 12:37 PM
abidmalikwaterloo edited the summary of this revision. (Show Details)
kiranchandramohan requested changes to this revision.Aug 10 2021, 10:47 AM

Thanks @abidmalikwaterloo for this patch. This patch needs a cleanup and should not have any unrelated changes. I have added some comments.

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

Nit: Alignment

500

You can remove private, firstprivate and lastprivate for now.

504

Nit: Alignment

607

Unrelated to this patch.

612–620

Unrelated to this patch.

mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
821–874

These are all unrelated to this patch. Please remove.

mlir/test/Dialect/OpenMP/ops.mlir
376–437

Removal of this is unrelated to this patch.

This revision now requires changes to proceed.Aug 10 2021, 10:47 AM
clementval requested changes to this revision.Aug 10 2021, 11:20 AM
clementval added inline comments.
mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
474

Line should be wrapped at 80.

475

Same here.

485

80

mlir/test/Dialect/OpenMP/ops.mlir
363

Unrelated to this patch

clementval added inline comments.Aug 10 2021, 11:29 AM
mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
504

Maybe too permissive for a chunk_size?

This patch has the same problem as the patch https://reviews.llvm.org/D105581/new/. The branching needs to be done from the main. The parsing part should not be there.

rebase the patch branch with the main branch

clementval requested changes to this revision.Sep 15 2021, 12:51 AM
clementval added inline comments.
mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
504

Alignment is still off.

This revision now requires changes to proceed.Sep 15 2021, 12:51 AM
abidmalikwaterloo marked 8 inline comments as done.Sep 17 2021, 7:32 AM

Made changes based on the reviewers' comments.

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

Any special reason for this?

504

We can make it AnyInteger. or IntORIndex

clementval added inline comments.Oct 5 2021, 12:39 AM
mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
507

Does it make sense to have a min value of 0. I guess if the collapse attr is present it value is greater than 0 and probably even greater than 1.

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

Any reason for removing them?

507

The loop operation is using the same value. However, I agreed that it should be >= 1.

clementval added inline comments.Oct 14 2021, 8:51 AM
mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
507

So do you want to keep 0 here?

Are we going to add pretty printers and parsers in a separate patch? Also, there are a few restrictions in the OpenMP reference, like

  • The region corresponding to the distribute construct must be strictly nested inside a teams region.
  • A list item may appear in a firstprivate or lastprivate clause but not both.

Maybe we can verify these (and others, if possible) too.

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

[suggestion] Maybe have it as a default valued attr with minimum of 1 like Confined<DefaultValuedAttr<I64Attr, "1">, [IntMinValue<1>]>.

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

Yes, I am thinking of this. However, I was thinking should we do the same for the parallel operations?

abidmalikwaterloo marked 4 inline comments as done.

change the collapse defualt value from 0 to 1

change the default collapse value from 0 to 1

clementval added inline comments.Oct 20 2021, 12:39 AM
mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
507

Yeah you can probably do it for the parallel op as well but it should be a separate patch.

abidmalikwaterloo marked an inline comment as done.Oct 22 2021, 6:30 AM
abidmalikwaterloo added inline comments.
mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
507

Will do.

Are you still working on this?

abidmalikwaterloo marked an inline comment as done.Jan 17 2022, 8:02 AM

I was busy. I will try to finalize this week.

peixin added a subscriber: peixin.Jan 18 2022, 4:07 AM
peixin added inline comments.
mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
467

Should this be 2.9.4.1 distribute Construct since you are implementing distribute construct rather than distribute loop constructs?

I have time now I will start looking them and would like finish them ASAP

Herald added a project: Restricted Project. · View Herald TranscriptMar 22 2022, 8:03 AM

Based on the feedback from other patches. This also needs an assembly format!

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

will update.

Added the assembly format, dist_schedule parser support

Herald added a project: Restricted Project. · View Herald TranscriptMay 12 2022, 8:32 AM

Thanks for working on this. A couple comments. There are no testcases. Please add testcases.

clang/lib/Testing/CMakeLists.txt
29 ↗(On Diff #428955)

unrelated change?

mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
488–490

If they are not added, please remove them from documentation too. It would be misleading otherwise.

514

dist_schedule is a dummy clause, where kind is only allowed to be static according to the standard. I don't think that requires a custom function, we can instead have something like the following -

arguments = UnitAttr:$static_dist_schedule, Optional<IntLikeType>:$schedule_chunk

assemblyFormat = `dist_schedule` `(` (`static` $static_dist_schedule^)? (`:` $schedule_chunk^)? `)`

Would that work? Let me know if there are any suggestions.

528

I think we need a verifier for this too. There are a couple semantic checks which we can do in verifier.

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

nit: schedule spelling.

shraiysh requested changes to this revision.May 18 2022, 2:15 AM
This revision now requires changes to proceed.May 18 2022, 2:15 AM
clang/lib/Testing/CMakeLists.txt
29 ↗(On Diff #428955)

When I rebase, these changes were highlighted in the main branch which was missing in the patch as it was too old.

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

My only concern is; will this be a dummy clause with the static scheduler forever? I am pretty sure dist_schedule will have a dynamic or a user defined scheduling strategy as well to improve the performance of a given application

abidmalikwaterloo marked 2 inline comments as done.May 18 2022, 11:28 AM
abidmalikwaterloo added inline comments.
mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
488–490

Ok

528

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
500

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

514

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.

528

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
514

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

528

At this stage, we can add:

LogicalResult DistributeOp::verify(){

return success();

}

We can add the check later!

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

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
514

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

528

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

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

Yup. We probably don't need these functions.

527

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
528

done

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

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
449

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
449

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
449

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
490

Do you mean static_dist_schedule here?

495–496

We can remove collapse for now.

513

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
211
220

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
124

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
449

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

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

Nit: Please make this change.

220

Nit: Please make this change as well.

mlir/test/Dialect/OpenMP/ops.mlir
124

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
124

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
124

Yes.

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

abidmalikwaterloo marked 2 inline comments as done.Jan 5 2023, 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.Jan 26 2023, 2:29 AM

Use omp.canonical_loop once it is in.

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