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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
