OpenMP distribute construct for MLIR operation and added a test case.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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. |
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.
mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td | ||
---|---|---|
504 | Alignment is still off. |
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 | ||
---|---|---|
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? |
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. |
mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td | ||
---|---|---|
507 | Will do. |
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? |
Based on the feedback from other patches. This also needs an assembly format!
mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td | ||
---|---|---|
467 | will update. |
Thanks for working on this. A couple comments. There are no testcases. Please add testcases.
clang/lib/Testing/CMakeLists.txt | ||
---|---|---|
29 | 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. |
clang/lib/Testing/CMakeLists.txt | ||
---|---|---|
29 | 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 |
clang/lib/Testing/CMakeLists.txt | ||
---|---|---|
29 | 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 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. |
clang/lib/Testing/CMakeLists.txt | ||
---|---|---|
29 | 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^)? ) | |
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? |
clang/lib/Testing/CMakeLists.txt | ||
---|---|---|
29 | 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. |
// 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?
Update the patch for assembly format. Change the WSLoopControl
to LoopControl. Added DistributeOp to parent of Yield OP. Added
one test case
clang/lib/Testing/CMakeLists.txt | ||
---|---|---|
30 | You still have these changes that are unrelated. |
mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td | ||
---|---|---|
449 |
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.
mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td | ||
---|---|---|
449 | What's the problem here? It seems fine to me. |
mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td | ||
---|---|---|
449 | syntax doesn't match the existing code. Change is highlighted. |
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. |
@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. |
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. |
Since the plan is to switch to the Canonical Style operation, may be this can wait till the omp.canonical_loop changes are in?
unrelated change?