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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
