TIFitis marked an inline comment as done.
TIFitis added inline comments.
================
Comment at:
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:1357
+/// Process MapOperands for Target Data directives.
+static LogicalResult processMapOperand(
+ llvm::IRBuilderBase &builder, LLVM::ModuleTranslation &moduleTranslation,
----------------
TIFitis wrote:
> kiranchandramohan wrote:
> > kiranchandramohan wrote:
> > > TIFitis wrote:
> > > > TIFitis wrote:
> > > > > kiranchandramohan wrote:
> > > > > > TIFitis wrote:
> > > > > > > kiranchandramohan wrote:
> > > > > > > > TIFitis wrote:
> > > > > > > > > kiranchandramohan wrote:
> > > > > > > > > > Isn't it possible to sink this whole function into the
> > > > > > > > > > OpenMPIRBuilder by passing it a list of `mapOpValue` and
> > > > > > > > > > `mapTypeFlags`?
> > > > > > > > > > `lvm::Value *mapOpValue =
> > > > > > > > > > moduleTranslation.lookupValue(mapOp);`
> > > > > > > > > >
> > > > > > > > > > Did i miss something? Or is this in anticipation of more
> > > > > > > > > > processing required for other types?
> > > > > > > > > I'm not fully sure but we might need more MLIR related things
> > > > > > > > > when supporting types other than LLVMPointerType. Also there
> > > > > > > > > is a call to mlir::LLVM::createMappingInformation.
> > > > > > > > >
> > > > > > > > > I guess it might still be possible to move most of it to the
> > > > > > > > > IRBuilder, would you like me to do that?
> > > > > > > > Callbacks are useful when there is frontend-specific handling
> > > > > > > > that is required. If more types require to be handled then it
> > > > > > > > is better to have the callback. We can revisit this after all
> > > > > > > > types are handled. I assume, the current handling is for
> > > > > > > > scalars and arrays of known-size.
> > > > > > > I am a novice at FORTRAN so I'm not aware of all the types and
> > > > > > > scenarios.
> > > > > > >
> > > > > > > I've tested the following cases and they work end-to-end:
> > > > > > >
> > > > > > > **Fortran:**
> > > > > > > ```
> > > > > > > subroutine openmp_target_data_region(a)
> > > > > > > real :: a(*)
> > > > > > > integer :: b(1024)
> > > > > > > character :: c
> > > > > > > integer, pointer :: p
> > > > > > > !$omp target enter data map(to: a, b, c, p)
> > > > > > > end subroutine openmp_target_data_region
> > > > > > > ```
> > > > > > >
> > > > > > > **LLVM IR(** `flang-new -fc1 -emit-llvm -fopenmp test.f90 -o
> > > > > > > test.ll`** ):**
> > > > > > >
> > > > > > > ```
> > > > > > > ; ModuleID = 'FIRModule'
> > > > > > > source_filename = "FIRModule"
> > > > > > > target datalayout =
> > > > > > > "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
> > > > > > > target triple = "x86_64-unknown-linux-gnu"
> > > > > > >
> > > > > > > %struct.ident_t = type { i32, i32, i32, i32, ptr }
> > > > > > >
> > > > > > > @0 = private unnamed_addr constant [13 x i8] c"loc(unknown)\00",
> > > > > > > align 1
> > > > > > > @1 = private unnamed_addr constant [56 x i8]
> > > > > > > c";/home/akash/Documents/scratch/test2.f90;unknown;3;16;;\00",
> > > > > > > align 1
> > > > > > > @2 = private unnamed_addr constant [56 x i8]
> > > > > > > c";/home/akash/Documents/scratch/test2.f90;unknown;4;18;;\00",
> > > > > > > align 1
> > > > > > > @3 = private unnamed_addr constant [56 x i8]
> > > > > > > c";/home/akash/Documents/scratch/test2.f90;unknown;5;25;;\00",
> > > > > > > align 1
> > > > > > > @4 = private unnamed_addr constant [23 x i8]
> > > > > > > c";unknown;unknown;0;0;;\00", align 1
> > > > > > > @5 = private unnamed_addr constant %struct.ident_t { i32 0, i32
> > > > > > > 2, i32 0, i32 22, ptr @4 }, align 8
> > > > > > > @.offload_maptypes = private unnamed_addr constant [4 x i64] [i64
> > > > > > > 1, i64 1, i64 1, i64 1]
> > > > > > > @.offload_mapnames = private constant [4 x ptr] [ptr @0, ptr @1,
> > > > > > > ptr @2, ptr @3]
> > > > > > >
> > > > > > > declare ptr @malloc(i64)
> > > > > > >
> > > > > > > declare void @free(ptr)
> > > > > > >
> > > > > > > define void @openmp_target_data_region_(ptr %0) {
> > > > > > > %2 = alloca [4 x ptr], align 8
> > > > > > > %3 = alloca [4 x ptr], align 8
> > > > > > > %4 = alloca [4 x i64], align 8
> > > > > > > %5 = alloca [1024 x i32], i64 1, align 4
> > > > > > > %6 = alloca [1 x i8], i64 1, align 1
> > > > > > > %7 = alloca { ptr, i64, i32, i8, i8, i8, i8 }, i64 1, align 8
> > > > > > > %8 = alloca ptr, i64 1, align 8
> > > > > > > store ptr null, ptr %8, align 8
> > > > > > > br label %entry
> > > > > > >
> > > > > > > entry: ; preds = %1
> > > > > > > %9 = getelementptr inbounds [4 x ptr], ptr %2, i32 0, i32 0
> > > > > > > store ptr %0, ptr %9, align 8
> > > > > > > %10 = getelementptr inbounds [4 x ptr], ptr %3, i32 0, i32 0
> > > > > > > store ptr %0, ptr %10, align 8
> > > > > > > %11 = getelementptr inbounds [4 x i64], ptr %4, i32 0, i32 0
> > > > > > > store i64 ptrtoint (ptr getelementptr (ptr, ptr null, i32 1) to
> > > > > > > i64), ptr %11, align 8
> > > > > > > %12 = getelementptr inbounds [4 x ptr], ptr %2, i32 0, i32 1
> > > > > > > store ptr %5, ptr %12, align 8
> > > > > > > %13 = getelementptr inbounds [4 x ptr], ptr %3, i32 0, i32 1
> > > > > > > store ptr %5, ptr %13, align 8
> > > > > > > %14 = getelementptr inbounds [4 x i64], ptr %4, i32 0, i32 1
> > > > > > > store i64 ptrtoint (ptr getelementptr (ptr, ptr null, i32 1) to
> > > > > > > i64), ptr %14, align 8
> > > > > > > %15 = getelementptr inbounds [4 x ptr], ptr %2, i32 0, i32 2
> > > > > > > store ptr %6, ptr %15, align 8
> > > > > > > %16 = getelementptr inbounds [4 x ptr], ptr %3, i32 0, i32 2
> > > > > > > store ptr %6, ptr %16, align 8
> > > > > > > %17 = getelementptr inbounds [4 x i64], ptr %4, i32 0, i32 2
> > > > > > > store i64 ptrtoint (ptr getelementptr (ptr, ptr null, i32 1) to
> > > > > > > i64), ptr %17, align 8
> > > > > > > %18 = getelementptr inbounds [4 x ptr], ptr %2, i32 0, i32 3
> > > > > > > store ptr %7, ptr %18, align 8
> > > > > > > %19 = getelementptr inbounds [4 x ptr], ptr %3, i32 0, i32 3
> > > > > > > store ptr %7, ptr %19, align 8
> > > > > > > %20 = getelementptr inbounds [4 x i64], ptr %4, i32 0, i32 3
> > > > > > > store i64 ptrtoint (ptr getelementptr (ptr, ptr null, i32 1) to
> > > > > > > i64), ptr %20, align 8
> > > > > > > %21 = getelementptr inbounds [4 x ptr], ptr %2, i32 0, i32 0
> > > > > > > %22 = getelementptr inbounds [4 x ptr], ptr %3, i32 0, i32 0
> > > > > > > %23 = getelementptr inbounds [4 x i64], ptr %4, i32 0, i32 0
> > > > > > > call void @__tgt_target_data_begin_mapper(ptr @5, i64 -1, i32
> > > > > > > 4, ptr %21, ptr %22, ptr %23, ptr @.offload_maptypes, ptr
> > > > > > > @.offload_mapnames, ptr null)
> > > > > > > ret void
> > > > > > > }
> > > > > > >
> > > > > > > ; Function Attrs: nounwind
> > > > > > > declare void @__tgt_target_data_begin_mapper(ptr, i64, i32, ptr,
> > > > > > > ptr, ptr, ptr, ptr, ptr) #0
> > > > > > >
> > > > > > > ; Function Attrs: nounwind
> > > > > > > declare void @__tgt_target_data_end_mapper(ptr, i64, i32, ptr,
> > > > > > > ptr, ptr, ptr, ptr, ptr) #0
> > > > > > >
> > > > > > > attributes #0 = { nounwind }
> > > > > > >
> > > > > > > !llvm.module.flags = !{!0}
> > > > > > >
> > > > > > > !0 = !{i32 2, !"Debug Info Version", i32 3}
> > > > > > > ```
> > > > > > >
> > > > > > >
> > > > > > > If I am missing some important types here then please let me
> > > > > > > know, I'll try to see if they work and if not I'll add support
> > > > > > > for them in further patches.
> > > > > > In general how are you passing the size of the fortran
> > > > > > variable/type to the OpenMP runtime? For scalars and arrays with
> > > > > > sizes known at compile time, this comes from the type itself. But
> > > > > > for other types like assumed-shape arrays, variable length arrays
> > > > > > this information comes from the descriptor or from other fields. My
> > > > > > question is how is this being collected and passed to the runtime?
> > > > > >
> > > > > > For all the types, I see the following code in the IR you gave
> > > > > > above for generating the `ArgSizes` argument of
> > > > > > `__tgt_target_data_begin_mapper`. I don't understand how the code
> > > > > > (and size) be the same for all the types.
> > > > > > ```
> > > > > > ...
> > > > > > %11 = getelementptr inbounds [4 x i64], ptr %4, i32 0, i32 0
> > > > > > store i64 ptrtoint (ptr getelementptr (ptr, ptr null, i32 1) to
> > > > > > i64), ptr %11, align 8
> > > > > > ...
> > > > > > %14 = getelementptr inbounds [4 x i64], ptr %4, i32 0, i32 1
> > > > > > store i64 ptrtoint (ptr getelementptr (ptr, ptr null, i32 1) to
> > > > > > i64), ptr %14, align 8
> > > > > > ...
> > > > > > %17 = getelementptr inbounds [4 x i64], ptr %4, i32 0, i32 2
> > > > > > store i64 ptrtoint (ptr getelementptr (ptr, ptr null, i32 1) to
> > > > > > i64), ptr %17, align 8
> > > > > > ...
> > > > > > %20 = getelementptr inbounds [4 x i64], ptr %4, i32 0, i32 3
> > > > > > store i64 ptrtoint (ptr getelementptr (ptr, ptr null, i32 1) to
> > > > > > i64), ptr %20, align 8
> > > > > > ...
> > > > > > %23 = getelementptr inbounds [4 x i64], ptr %4, i32 0, i32 0
> > > > > > call void @__tgt_target_data_begin_mapper(ptr @5, i64 -1, i32 4,
> > > > > > ptr %21, ptr %22, ptr %23, ptr @.offload_maptypes, ptr
> > > > > > @.offload_mapnames, ptr null)
> > > > > > ```
> > > > > >
> > > > > > I would like some more clarity on this before proceeding. Clang
> > > > > > generates different code for this and I see that it is
> > > > > > appropriately filling the `ArgSizes` field.
> > > > > `OpenMPIRBuilder::getSizeInBytes` is the function responsible for
> > > > > calculating the `ArgSizes`.
> > > > >
> > > > > For the Value : `%1 = alloca i64, i64 1, align 8` it returns size as
> > > > > `i64 ptrtoint (ptr getelementptr (ptr, ptr null, i32 1) to i64)` and
> > > > > TBH I don't understand this. This function was taken from OpenACC.
> > > > >
> > > > > I will re-implement this function and update the patch.
> > > > Actually, the generated code seems correct. The first answer [[
> > > > https://stackoverflow.com/questions/14608250/how-can-i-find-the-size-of-a-type
> > > > | here ]] gives insight into how `OpenMPIRBuilder::getSizeInBytes` is
> > > > calculating the size of the type.
> > > >
> > > > Opaque pointers make it look the same for all the different types,
> > > > disabling opaque pointers you get something like the following:
> > > >
> > > > `integer(8) :: a` :
> > > > ```
> > > > %7 = getelementptr inbounds [1 x i64], [1 x i64]* %.offload_sizes,
> > > > i32 0, i32 0
> > > > store i64 ptrtoint (i64** getelementptr (i64*, i64** null, i32 1) to
> > > > i64), i64* %7, align 4
> > > > ```
> > > >
> > > > `integer :: b(1024)` :
> > > >
> > > > ```
> > > > %8 = getelementptr inbounds [1 x i64], [1 x i64]* %.offload_sizes,
> > > > i32 0, i32 0
> > > > store i64 ptrtoint ([1024 x i32]** getelementptr ([1024 x i32]*,
> > > > [1024 x i32]** null, i32 1) to i64), i64* %8, align 4
> > > > ```
> > > >
> > > > Let me know if this makes sense.
> > > >
> > > > Thanks,
> > > > Akash
> > > Ahh OK. It does make it a bit harder to read.
> > >
> > > But going to back to my general question:
> > > ```
> > > In general how are you passing the size of the fortran variable/type to
> > > the OpenMP runtime? For scalars and arrays with sizes known at compile
> > > time, this comes from the type itself. But for other types like
> > > assumed-shape arrays, variable length arrays this information comes from
> > > the descriptor or from other fields. My question is how is this being
> > > collected and passed to the runtime
> > > ```
> > >
> > > Consider the following two subroutines, It has two assumed shape arrays.
> > > In sb0 it is a rank-1 array, in sb1 it is a rank-2 array. At the llvm
> > > dialect layer, these two will be represented by struct equivalents of
> > > Fortran descriptors as given below. If we now find the size of the types,
> > > it would get the size of the descriptor struct rather than it memory it
> > > is referring to. I guess this is not what we want to do. I believe this
> > > would require some special processing, unless the patch also does
> > > something for this.
> > >
> > > ```
> > > omp.target_data map((to -> %arg0 : !llvm.ptr<struct<(ptr<i32>, i64,
> > > i32, i8, i8, i8, i8, array<1 x array<3 x i64>>)>>))
> > > ```
> > >
> > > ```
> > > omp.target_data map((to -> %arg0 : !llvm.ptr<struct<(ptr<i32>, i64,
> > > i32, i8, i8, i8, i8, array<1 x array<3 x i64>>)>>))
> > > ```
> > >
> > >
> > > ```
> > > subroutine sb0(a)
> > > integer :: a(:)
> > > !$omp target data map(to: a)
> > > a(10) = 20
> > > !$omp end target data
> > > end subroutine
> > >
> > > subroutine sb1(a)
> > > integer :: a(:,:)
> > > !$omp target data map(to: a)
> > > a(5,6) = 20
> > > !$omp end target data
> > > end subroutine
> > > ```
> > Just want to clarify that I am not expecting a fix here. But just a
> > statement about what is supported and what is not supported.
> In line 1382 you can see how the size is determined. It is done simply by
> passing the map variable to the `getSizeInBytes` function.
>
> I haven't added any special handling for assumed-shape arrays or any other
> special cases. I am not sure if it is equivalent to an `int*` in C, but
> looking at the llvm IR, if the map variable is an `i32*` then clang sets the
> size to 4, and for assumed-shape arrays in your examples `sb0` and `sb1` it
> also creates `i32*` in the llvm IR and the size for these calculated in the
> same way.
>
> I am not sure if this is the correct behaviour. Do you know what is the
> correct size that should be passed in your examples to the runtime? If it is
> relatively straightforward then I can look into adding support for these,
> otherwise maybe we can add a TODO for handling assumed-shape arrays in a
> future patch.
Sorry, made a mistake in previous comment.
Clang sets size to 8 and not 4 for `i32*`.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D142914/new/
https://reviews.llvm.org/D142914
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
