TIFitis marked 3 inline comments as done.
TIFitis added a comment.
> 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
}
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.
================
Comment at: mlir/lib/Dialect/OpenMP/CMakeLists.txt:15
MLIRLLVMDialect
+ MLIRArithDialect
)
----------------
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.
================
Comment at: mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp:584
+ std::stringstream typeMod, type;
+ if (mapTypeBits & 0x04)
+ typeMod << "always ";
----------------
kiranchandramohan wrote:
> There is a `bitn` function in `printSynchronizationHint` and
> `verifySynchronizationHint`, can that be used here?
I've added something similar. I got the bit values from Clang codegen and they
use hex so I've kept it that way for uniformity. Let me know if you'd rather it
be in n'th bit format.
================
Comment at: mlir/test/Dialect/OpenMP/ops.mlir:389
+ // CHECK: omp.target_data
+ "omp.target_data"(%if_cond, %device, %data1, %data2) ({
+
----------------
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.
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
