TIFitis marked 5 inline comments as done. TIFitis added a comment. In D131915#4056730 <https://reviews.llvm.org/D131915#4056730>, @kiranchandramohan wrote:
> LGTM. Thanks for making the changes and for your patience. Please wait a > couple of days to give other reviewers a chance to have a look before > submitting. Thanks for reviewing the changes as well :) > Could you update the Summary of this patch to specify what is in this patch > and what is left out? Also, might be useful to specify any special modelling, > like for the map clause. I've updated the summary. The new map clause looks straight forward to me, anything particular you want me to mention in the summary? > Could you specify the co-authors in the following way? > Co-authored-by: abidmalikwaterloo <ama...@bnl.gov> > Co-authored-by: raghavendra <raghu.maddhipa...@amd.com> Done. Cheers, Akash ================ Comment at: mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td:790 +//===---------------------------------------------------------------------===// +// 2.12.2 target data Construct +//===---------------------------------------------------------------------===// ---------------- kiranchandramohan wrote: > Is this number from the OpenMP 5.0 standard? I think 5.0 does not have the > `present` map-type modifier. The printer includes this. I think we can either > remove things that are not there in 5.0 or add comments when items from newer > versions are included or alternatively change the number to the latest > version and call out everything that is not implemented. Hi I've updated it to OpenMP 5.1 standard specification. We are missing the `depend` clause, and mapper and iterator values for the map_type_modifier which are already specified in the TODO. ================ Comment at: mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp:542 +// Parser, printer and verifier for Target Data +//===----------------------------------------------------------------------===// +static ParseResult ---------------- kiranchandramohan wrote: > Could you specify the EBNF (https://mlir.llvm.org/docs/LangRef/#notation) for > the expected structure of the map clause? Now that we are using `IntegerAttr` we no longer need the Attr to be explicitly present when printing it. I have thus updated the printer and parser accordingly, also removed `none` as a map_type_modifier as absence of other modifiers implicitly implies it and prevents confusion by diverging from the specification. Here's an example of the new custom printer: ``` omp.target_exit_data map((release -> %1 : !llvm.ptr<array<1024 x i32>>), (always, close, from -> %2 : !llvm.ptr<array<1024 x i32>>)) ``` Let me know if the EBNF is okay. Should I override clang-format and keep the grammar and Eg in the same line to make them easier to read? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D131915/new/ https://reviews.llvm.org/D131915 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
