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

[Akash Banerjee via Phabricator via cfe-commits]

TIFitis marked 4 inline comments as done.
TIFitis added inline comments.


================
Comment at: mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td:829
+              VariadicOfVariadic<AnyType, "map_operand_segments">:$map_operands,
+              DenseI32ArrayAttr:$map_operand_segments);
+
----------------
kiranchandramohan wrote:
> Is `map_operand_segments` currently unused?
`map_operand_segments` is required by VariadicOfVariadic type to keep track of the dimensions. I haven't made explicit use of it, however, it is used internally and cant be avoided AFAIK. 

It is akin to the `operand_segment_sizes` which is implicitly present for Operations with `Optional` or `Variadic` operands.


================
Comment at: mlir/lib/Dialect/OpenMP/CMakeLists.txt:15
   MLIRLLVMDialect
+  MLIRArithDialect
   )
----------------
kiranchandramohan wrote:
> TIFitis wrote:
> > kiranchandramohan wrote:
> > > Why is this needed here?
> > The Arith Dialect needs to be linked against as we're using it extract the int value from arith.constant in the custom printer.
> Can this be avoided by modelling constants as attributes?
The issue with attributes is AFAIK `Variadic<Attribute>` is not supported, and as previously discussed we need it be `Variadic` to support multiple map clauses.

If I am wrong and there is indeed a way to have `Variadic<Attribute>` then this can be avoided.


================
Comment at: mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp:577-583
+    if (auto constOp =
+            mlir::dyn_cast<mlir::arith::ConstantOp>(a.front().getDefiningOp()))
+      mapTypeBits = constOp.getValue()
+                        .cast<mlir::IntegerAttr>()
+                        .getValue()
+                        .getSExtValue();
+    else if (auto constOp = mlir::dyn_cast<mlir::LLVM::ConstantOp>(
----------------
kiranchandramohan wrote:
> Generally, constant values are modelled as attributes in MLIR representation. Can we switch to that representation? This will also avoid the need for this `if-else` and dependence on the `arith` dialect.
Copy:
The issue with attributes is AFAIK `Variadic<Attribute>` is not supported, and as previously discussed we need it be `Variadic` to support multiple map clauses.

If I am wrong and there is indeed a way to have `Variadic<Attribute>` then this can be avoided.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131915