[PATCH] D142914: [MLIR][OpenMP] Added OMPIRBuilder support for Target Data directives.
Kiran Chandramohan via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Wed Mar 8 08:25:49 PST 2023
kiranchandramohan requested changes to this revision.
kiranchandramohan added inline comments.
This revision now requires changes to proceed.
================
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:
> > 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.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D142914/new/
https://reviews.llvm.org/D142914
More information about the cfe-commits
mailing list