shraiysh added inline comments.
================
Comment at: clang/lib/Testing/CMakeLists.txt:29
llvm_gtest
+ clangBasic
+ clangFrontend
----------------
abidmalikwaterloo wrote:
> shraiysh wrote:
> > 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.
> git rebase origin arcpatch-D105584 gives " The current branch is up to date".
> It means the patch is up to date. Should I remove them manually?
I'm not sure what the issue is, but this change should not be reflected here if
the patch is properly rebased with main.
================
Comment at: mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td:514
+ oilist(`schedule` `(`
+ custom<DistributeScheduleClause>(
+ $dist_schedule_var,
----------------
abidmalikwaterloo wrote:
> shraiysh wrote:
> > 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.
> Should it be then? `dist_schedule` `(` (`static` ) (`:` $schedule_chunk^)?
> `)`
> $static_dist_schedule seems redundant???
AFAIK, the keyword `static` is optional. Without the presence of
`$static_dist_schedule`, how are you going to store this flag?
================
Comment at: mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td:528
+
+ let regions = (region AnyRegion:$region);
+}
----------------
abidmalikwaterloo wrote:
> shraiysh wrote:
> > 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.
> At this stage, we can add:
>
> LogicalResult DistributeOp::verify(){
> return success();
> }
>
> We can add the check later!
Why not add the check now itself. Can it not be implemented? It's a small check
:/
================
Comment at: mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp:226
+ return parser.emitError(parser.getNameLoc()) <<
+ " expected scheudle kind";
+
----------------
abidmalikwaterloo wrote:
> shraiysh wrote:
> > nit: schedule spelling.
> I do not think we need this if we treat the schedule as a variable?
Yup. We probably don't need these functions.
================
Comment at: mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp:565
//===----------------------------------------------------------------------===//
// WsLoopOp
//===----------------------------------------------------------------------===//
----------------
Change this comment to Loop control.
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
