[PATCH] D131915: [MLIR][OpenMP] Added target data, exit data, and enter data operation definition for MLIR.
Kiran Chandramohan via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Fri Dec 16 05:08:49 PST 2022
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
More information about the cfe-commits
mailing list