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

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



================
Comment at: mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td:490
+    
+    The optional `dist_schedule_var` attribute specifies the  schedule for this
+    loop, determining how the loop is distributed across the parallel threads.
----------------
Do you mean `static_dist_schedule` here?


================
Comment at: mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td:495-496
+
+    The optional `collapse` attribute specifies the number of loops which
+    are collapsed to form the distribute loop.
+  }];
----------------
We can remove `collapse` for now.


================
Comment at: mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td:513
+    		( $schedule_chunk^ `:` type($schedule_chunk))? `)`
+          |`collapse` `(` $collapse_val `)`
+          | `allocate` `(`
----------------
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.


================
Comment at: mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp:211
+//===---------------------------------------------------------------------===//
+// Verifier for Dristribute Op
+//===---------------------------------------------------------------------===//
----------------



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


================
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) -> () {
----------------
Add a pretty-print test as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105584