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

[Shraiysh via Phabricator via cfe-commits]

shraiysh added inline comments.


================
Comment at: clang/lib/Testing/CMakeLists.txt:29
   llvm_gtest
+  clangBasic
+  clangFrontend
----------------
abidmalikwaterloo wrote:
> shraiysh wrote:
> > unrelated change?
> When I rebase, these changes were highlighted in the main branch which was missing in the patch as it was too old.
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.


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


================
Comment at: mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td:528
+
+  let regions = (region AnyRegion:$region);
+}
----------------
abidmalikwaterloo wrote:
> shraiysh wrote:
> > I think we need a verifier for this too. There are a couple semantic checks which we can do in verifier.
> Can you say more about the semantic checks you have in mind?
The following restriction from the standard can be added to the verifier/Operation definition - 
> The region corresponding to the distribute construct must be strictly nested inside a teams region.

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.


================
Comment at: mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td:265
+             Variadic<IntLikeType>:$step,
+	     Variadic<AnyType>:$private_var,
+             Variadic<AnyType>:$firstprivate_var,
----------------
abidmalikwaterloo wrote:
> abidmalikwaterloo wrote:
> > kiranchandramohan wrote:
> > > kiranchandramohan wrote:
> > > > Nit: Alignment
> > > You can remove private, firstprivate and lastprivate for now.
> > Any special reason for this?
> Any reason for removing them? 
They are being removed because we do not have a clean way of handling these clauses in MLIR for any generic frontend.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105584