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

[Shraiysh via Phabricator via cfe-commits]

shraiysh added a comment.

Minor nits about documentation.

LGTM given that verifier and assembly format will be added soon for each of these as a separate patch soon. Please wait for atleast one more +1.

The OpenMP IRBuilder work does not have to wait for this patch to merge. You can start working on that while keeping this patch as a dependency.



================
Comment at: clang/lib/Testing/CMakeLists.txt:6
+=======
+>>>>>>> [Testing] TestAST, a helper for writing straight-line AST tests
 # Not add_clang_library: this is not part of clang's public library interface.
----------------
nit: Merge conflict. Please remove.


================
Comment at: mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td:594
+
+    The $map_operands specifies operands with no map type
+
----------------
Data mapping is very similar to data privatization. Would it be a good idea to push this to the frontends while we have not concretely decided on openmp dialect privatization/mapping? If you have an idea on how to implement mapping in OpenMP Dialect to LLVM IR maybe we can go ahead with this.

CC @kiranchandramohan for suggestions regarding privatization.


================
Comment at: mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td:661
+
+    The $map_type_modifier vector specifies the modifier for each map type 
+    operand
----------------
It is not clear which element in this list corresponds to which map type. It would be nice if we could either document that here, or have separate operands for each map type. (same for other two constructs too)


================
Comment at: mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td:267
+
+    The optional $use_device_ptr specifies the device pointers to the 
+    corresponding list items in the device data environment
----------------
shraiysh wrote:
> please put variables in documentation within backticks(`). This is because when the documentation is rendered on mlir.llvm.org, $ and _ interact to give unexpected text if backticks are not used.
nit: Ping on this comment. The variable names have not been put in backticks and this is going to cause issues with autogenerated documentation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105255