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, ---------------- 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. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D142914/new/ https://reviews.llvm.org/D142914 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits