OpenMP distribute construct for MLIR operation and added a test case.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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 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 ↗ | (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^)? ) | |
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? |
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. |
// 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 ↗ | (On Diff #442321) | You still have these changes that are unrelated. |
mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td | ||
---|---|---|
455 |
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 | ||
---|---|---|
455 | What's the problem here? It seems fine to me. |
mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td | ||
---|---|---|
455 | 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 | ||
---|---|---|
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. |
@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. |
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. |
Since the plan is to switch to the Canonical Style operation, may be this can wait till the omp.canonical_loop changes are in?