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

[Shraiysh via Phabricator via cfe-commits]

shraiysh added a comment.

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



================
Comment at: clang/lib/Testing/CMakeLists.txt:29
   llvm_gtest
+  clangBasic
+  clangFrontend
----------------
unrelated change?


================
Comment at: mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td:488-490
+    `private_var`, `firstprivate_var`, and `lastprivate_var` arguments are 
+    variadic list of operands that specify the data sharing attributes of the 
+    list of values 
----------------
If they are not added, please remove them from documentation too. It would be misleading otherwise.


================
Comment at: mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td:514
+    oilist(`schedule` `(`
+              custom<DistributeScheduleClause>(
+               $dist_schedule_var,
----------------
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.


================
Comment at: mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td:528
+
+  let regions = (region AnyRegion:$region);
+}
----------------
I think we need a verifier for this too. There are a couple semantic checks which we can do in verifier.


================
Comment at: mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp:226
+    	return parser.emitError(parser.getNameLoc()) <<
+    				" expected scheudle kind";
+    
----------------
nit: schedule spelling.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105584