[PATCH] D105584: [MLIR][OpenMP] Distribute Construct Operation

[Kiran Chandramohan via Phabricator via cfe-commits]

kiranchandramohan requested changes to this revision.
kiranchandramohan added a comment.
This revision now requires changes to proceed.

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



================
Comment at: mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td:449
      ParentOneOf<["WsLoopOp", "ReductionDeclareOp", 
-     "AtomicUpdateOp", "SimdLoopOp"]>]> {
+     "AtomicUpdateOp", "SimdLoopOp","DistributeOp"]>]> {
   let summary = "loop yield and termination operation";
----------------
clementval wrote:
> abidmalikwaterloo wrote:
> > clementval wrote:
> > > 
> > What's the problem here? It seems fine to me.
> syntax doesn't match the existing code. Change is highlighted.
Nit: Please fix this (adding a space before `"DistributeOp"`).


================
Comment at: mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp:211
+//===---------------------------------------------------------------------===//
+// Verifier for Dristribute Op
+//===---------------------------------------------------------------------===//
----------------
kiranchandramohan wrote:
> 
Nit: Please make this change.


================
Comment at: mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp:220
+    return emitOpError() << "empty upperbound for distribute loop operation";
+  
+  return success();	
----------------
kiranchandramohan wrote:
> Nit: Check that their sizes are equal as well. And also if step is present then its size matches lowerbound/upperbound.
Nit: Please make this change as well.


================
Comment at: mlir/test/Dialect/OpenMP/ops.mlir:124
 
+// CHECK-LABEL: omp_DistributeOp
+func.func @omp_DistributeOp(%lb : index, %ub : index, %step : index, %data_var : memref<i32>, %chunk_var : i32) -> () {
----------------
kiranchandramohan wrote:
> Add a pretty-print test as well.
Nit: please add a pretty-print test.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D105584/new/

https://reviews.llvm.org/D105584