abidmalikwaterloo marked an inline comment as done.
abidmalikwaterloo added inline comments.
================
Comment at: clang/lib/Testing/CMakeLists.txt:29
llvm_gtest
+ clangBasic
+ clangFrontend
----------------
shraiysh wrote:
> 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.
I will remove them manually. I think rebase is correct.
================
Comment at: mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td:528
+
+ let regions = (region AnyRegion:$region);
+}
----------------
shraiysh wrote:
> 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 :/
done
================
Comment at: mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp:226
+ return parser.emitError(parser.getNameLoc()) <<
+ " expected scheudle kind";
+
----------------
shraiysh wrote:
> 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.
done
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D105584/new/
https://reviews.llvm.org/D105584
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
