[PATCH] D131915: [MLIR][OpenMP] Added target data, exit data, and enter data operation definition for MLIR.

[Kiran Chandramohan via Phabricator via llvm-commits]

kiranchandramohan accepted this revision.
kiranchandramohan added a comment.
This revision is now accepted and ready to land.

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.

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.

Could you specify the co-authors in the following way?
Co-authored-by: abidmalikwaterloo <andrzej.warzynski at arm.com>
Co-authored-by: raghavendra <Raghu.Maddhipatla at amd.com>



================
Comment at: mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td:790
+//===---------------------------------------------------------------------===//
+// 2.12.2 target data Construct
+//===---------------------------------------------------------------------===//
----------------
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.


================
Comment at: mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td:818-820
+    The $map_operands specifies operands in map clause along with it's type and modifiers.
+
+    The $map_operand_segments specifies the segment sizes for $map_operands.
----------------
Update these two to their current meanings. (Also replace `map_operand_segments` with `map_types`.

Same comment for the other two operations.


================
Comment at: mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp:542
+// Parser, printer and verifier for Target Data
+//===----------------------------------------------------------------------===//
+static ParseResult
----------------
Could you specify the EBNF (https://mlir.llvm.org/docs/LangRef/#notation) for the expected structure of the map clause?


================
Comment at: mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp:595
+
+    if (mapTypeOp.isa<mlir::IntegerAttr>())
+      mapTypeBits = mapTypeOp.cast<mlir::IntegerAttr>().getInt();
----------------
Nit: Should this be an assert?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131915