kiranchandramohan requested changes to this revision. kiranchandramohan added a comment. This revision now requires changes to proceed.
Can you add the other authors as Co-authors in the patch Summary? In D131915#4001620 <https://reviews.llvm.org/D131915#4001620>, @TIFitis wrote: >> Can you add this as a test? AFAIS, the tests attached to this patch do not >> seem to be exercising the `VariadicofVariadic` requirement. An explanation >> with an example would be great. > > `VariadicOfVariadic` gives us a `SmallVector<ValueRange>` where `ValueRange` > is essentially a `SmallVector<Value>`. As it is possible to have multiple map > clauses for a single target construct, this allows us to represent each map > clause as a row in `$map_operands`. > > I've updated the ops.mlir test to use two map clauses which better shows this > use case. > >> 1. Fortran Source + OpenMP example that needs a VariadicOfVariadic >> 2. MLIR OpenMP representation > > Fortran Source: > > subroutine omp_target_enter > integer :: a(1024) > integer :: b(1024) > !$omp target enter data map(to: a,) map(always, alloc: b) > end subroutine omp_target_enter > > MLIR OpenMP representation: > > func.func @_QPomp_target_enter() { > %0 = fir.alloca !fir.array<1024xi32> {bindc_name = "a", uniq_name = > "_QFomp_target_enterEa"} > %1 = fir.alloca !fir.array<1024xi32> {bindc_name = "b", uniq_name = > "_QFomp_target_enterEb"} > %c1_i64 = arith.constant 1 : i64 > %c4_i64 = arith.constant 4 : i64 > omp.target_enter_data map((%c1_i64 -> none , to : %0 : > !fir.ref<!fir.array<1024xi32>>), (%c4_i64 -> always , alloc : %1 : > !fir.ref<!fir.array<1024xi32>>)) > return > } Thanks, that helps. > I plan on adding these as tests along with Fortran lowering support in > upcoming patches. > >> I am assuming we would need a verifier as well for the map clause. > > AFAIK the map clause rules are op specific. I plan on adding verifiers for > the ops soon in a separate patch. > >> Can you also restrict this patch to one of the constructs say `target data` >> for this patch? Once we decide on that then the other two can be easily >> added in a separate patch. > > Since I didn't make any changes to the ops I've left them in. If the patch > requires further changes, I'll leave them out. The concern is with the introduction of VariadicOfVariadic with `AnyType`. I think this is new in the OpenMP Dialect and it pretty much allows anything. So, I was thinking whether it would be a good idea to write a verifier for the map clause, if not for the entire construct. ================ Comment at: mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td:826-827 + Optional<AnyInteger>:$device, + Variadic<AnyType>:$use_device_ptr, + Variadic<AnyType>:$use_device_addr, + VariadicOfVariadic<AnyType, "map_operand_segments">:$map_operands, ---------------- This is OK for now, but we might have to switch to `OpenMPPointerType` later. ================ Comment at: mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td:829 + VariadicOfVariadic<AnyType, "map_operand_segments">:$map_operands, + DenseI32ArrayAttr:$map_operand_segments); + ---------------- Is `map_operand_segments` currently unused? ================ Comment at: mlir/lib/Dialect/OpenMP/CMakeLists.txt:15 MLIRLLVMDialect + MLIRArithDialect ) ---------------- 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? ================ 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>( ---------------- 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. ================ Comment at: mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp:595 + + std::stringstream typeMod, type; + if (always) ---------------- I think llvm prefers using `llvm::raw_string_ostream`. I have seen only two uses of std::stringstream in MLIR code, possibly accidentally introduced. ================ Comment at: mlir/test/Dialect/OpenMP/ops.mlir:389 + // CHECK: omp.target_data + "omp.target_data"(%if_cond, %device, %data1, %data2) ({ + ---------------- TIFitis wrote: > kiranchandramohan wrote: > > TIFitis wrote: > > > clementval wrote: > > > > Can you switch to the custom format form? > > > Hi, I've updated the test file, Is this what you wanted? > > Both the input and the CHECK lines in the custom format. > I've changed both to custom format. Added a second map clause argument to > show support. Can you increase the coverage so that each clause is present atleast once, probably by adding more than one test for each operation. 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
