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

[Abid via Phabricator via cfe-commits]

abidmalikwaterloo marked 2 inline comments as done.
abidmalikwaterloo added inline comments.


================
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.
----------------
shraiysh wrote:
> nit: Merge conflict. Please remove.
removed


================
Comment at: clang/lib/Testing/CMakeLists.txt:33
+  clangBasic
+  clangFrontend
   )
----------------
shraiysh wrote:
> abidmalikwaterloo wrote:
> > Are these okay? In D105584 these were not
> No, this is not okay. There is some issue with these patches and I don't know what it is, but these unrelated changes should not be seen here. Thank you for pointing this out :)
removed them manually


================
Comment at: mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td:594
+
+    The $map_operands specifies operands with no map type
+
----------------
shraiysh wrote:
> 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.
@kiranchandramohan @raghavendhra I would like to keep it like this! Any comments


================
Comment at: mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td:661
+
+    The $map_type_modifier vector specifies the modifier for each map type 
+    operand
----------------
shraiysh wrote:
> 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)
I feel keeping it in vector makes more sense. I will document it here. As per OpenMP Specifications:
The map-type can be **to, from, tofrom, or alloc**.


================
Comment at: mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td:718
+    operand
+
+    TODO: depend
----------------
will document map type here as wekk


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105255