kiranchandramohan requested changes to this revision. kiranchandramohan added a comment. This revision now requires changes to proceed.
Thanks for the changes and your work here. In D131915#3994160 <https://reviews.llvm.org/D131915#3994160>, @TIFitis wrote: > Added custom printer & parser for map clause. Updated ops.mlir test to > address reviewer comments. > > Custom Printer example: > > OMP: !$omp target exit data map(from: a,b) map(release: c) map(delete: d) > map(always, from: e) > CustomPrinter: omp.target_exit_data map((%c2_i64 -> none , from : %0 : > !fir.ref<!fir.array<1024xi32>>), (%c2_i64 -> none , from : %1 : > !fir.ref<!fir.array<1024xi32>>), (%c0_i64 -> none , release : %2 : > !fir.ref<!fir.array<1024xi32>>), (%c8_i64 -> none , delete : %3 : > !fir.ref<!fir.array<1024xi32>>), (%c6_i64 -> always , from : %4 : > !fir.ref<!fir.array<1024xi32>>)) 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. 1. Fortran Source + OpenMP example that needs a VariadicOfVariadic 2. MLIR OpenMP representation I am assuming we would need a verifier as well for the map clause. 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. ================ Comment at: mlir/lib/Dialect/OpenMP/CMakeLists.txt:15 MLIRLLVMDialect + MLIRArithDialect ) ---------------- Why is this needed here? ================ Comment at: mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp:572 + for (const auto &a : map_operands) { + auto mapTypeBits = 0x00; + if (auto constOp = ---------------- Nit: can you specify the type here? ================ Comment at: mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp:584 + std::stringstream typeMod, type; + if (mapTypeBits & 0x04) + typeMod << "always "; ---------------- There is a `bitn` function in `printSynchronizationHint` and `verifySynchronizationHint`, can that be used here? ================ Comment at: mlir/test/Dialect/OpenMP/ops.mlir:389 + // CHECK: omp.target_data + "omp.target_data"(%if_cond, %device, %data1, %data2) ({ + ---------------- 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. 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
