TIFitis added inline comments.
================ Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:1561 + /// should be placed. + /// \param HasRegion True if the op has a region associated with it, false + /// otherwise ---------------- kiranchandramohan wrote: > TIFitis wrote: > > kiranchandramohan wrote: > > > Can't the presence/absence of the BodyGenCB indicate the presence/absence > > > of a region? > > The `BodyGenCB` is always present as an argument right? Passing a `nullptr` > > when its not required doesn't seem possible at least when using the > > `BodyGenCallbackTy`. Is there a way to selectively pass the `BodyGenCB`? > The only optional CallBack seems to be the `FinalizeCallbackTy`. It is > defined as an `std::function` whereas `BodyGenCallBackTy` is defined as a > `function_ref`. But the definition of `function_ref` looks like it can be > used to check whether it is a `nullptr` or not. Did you face any issues in > trying to make it optional with a default parameter value of `nullptr`? > > https://llvm.org/doxygen/STLFunctionalExtras_8h_source.html#l00036 > > ``` > void emitCancelationCheckImpl(Value *CancelFlag, > omp::Directive CanceledDirective, > FinalizeCallbackTy ExitCB = {}); > ``` Sorry, I was being stupid and trying to pass `nullptr` as an argument instead of making the actual parameter optional. `BodyGenCallBackTy` works. ================ Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:4052-4060 + // LLVM utilities like blocks with terminators. + auto *UI = Builder.CreateUnreachable(); + Instruction *ThenTI = UI, *ElseTI = nullptr; + if (IfCond) { + SplitBlockAndInsertIfThenElse(IfCond, UI, &ThenTI, &ElseTI); + ThenTI->getParent()->setName("omp_if.then"); + ElseTI->getParent()->setName("omp_if.else"); ---------------- kiranchandramohan wrote: > TIFitis wrote: > > kiranchandramohan wrote: > > > TIFitis wrote: > > > > kiranchandramohan wrote: > > > > > There is some recent recommendation to not insert artificial > > > > > instructions and remove them. The preferred approach is to use > > > > > `splitBB` function(s) inside the OpenMPIRBuilder. This function works > > > > > on blocks without terminators. You can consult the `IfCondition` code > > > > > in `createParallel`. > > > > Thanks a lot for taking the time to review this lengthy patch. > > > > > > > > This one seems a bit tricky to do. At first glance `createParallel` > > > > seems to be doing something different where its calling different > > > > runtime functions based on the `IfCondition` instead of much in the way > > > > of Control Flow changes. > > > > > > > > The `unreachable` inst helps out a lot here as it makes it really easy > > > > to keep trace of the resume point for adding instructions after the > > > > `BodyGen` codes are generated. > > > > > > > > I am still looking into finding a way to do this elegantly without > > > > having to use the `unreachable` instruction, but would it be a major > > > > blocker if we had to keep it? > > > > > > > > I have addressed all the other changes you requested. > > > Thanks for explaining the need for the `unreachable`. I will leave it > > > with you on whether to make the change. > > > > > > You can see an instance of a past request for not using temporary > > > instruction. That patch (if for createTask) addressed the issue one way. > > > https://reviews.llvm.org/D130615#inline-1257711 > > > > > > I gave a quick try and came up with the following code. I don't think it > > > is very elegant, but it is one way to do it. > > > ``` > > > - // LLVM utilities like blocks with terminators. > > > - auto *UI = Builder.CreateUnreachable(); > > > + BasicBlock *ContBB = nullptr; > > > if (IfCond) { > > > - auto *ThenTI = > > > - SplitBlockAndInsertIfThen(IfCond, UI, /* Unreachable */ false); > > > - ThenTI->getParent()->setName("omp_if.then"); > > > - Builder.SetInsertPoint(ThenTI); > > > - } else { > > > - Builder.SetInsertPoint(UI); > > > + BasicBlock *EntryBB = Builder.GetInsertBlock(); > > > + ContBB = splitBB(Builder, /*CreateBranch*/ false); > > > + BasicBlock *ThenBB = > > > + BasicBlock::Create(Builder.getContext(), EntryBB->getName() + ".if", > > > + ContBB->getParent(), ContBB); > > > + ContBB->setName(EntryBB->getName() + ".if.end"); > > > + Builder.CreateCondBr(IfCond, ThenBB, ContBB); > > > + Builder.SetInsertPoint(ThenBB); > > > + Builder.CreateBr(ContBB); > > > + Builder.SetInsertPoint(ThenBB->getTerminator()); > > > } > > > > > > ProcessMapOpCB(Builder.saveIP(), Builder.saveIP()); > > > @@ -4087,9 +4090,10 @@ OpenMPIRBuilder::InsertPointTy > > > OpenMPIRBuilder::createTargetData( > > > emitMapperCall(Builder.saveIP(), beginMapperFunc, srcLocInfo, > > > MapTypesArg, > > > MapNamesArg, MapperAllocas, DeviceID, > > > MapTypeFlags.size()); > > > > > > + BasicBlock *DataExitBB = splitBB(Builder, true, "target_data.exit"); > > > BodyGenCB(Builder.saveIP(), Builder.saveIP()); > > > > > > - Builder.SetInsertPoint(UI->getParent()); > > > + Builder.SetInsertPoint(DataExitBB, DataExitBB->begin()); > > > // Create call to end the data region. > > > emitMapperCall(Builder.saveIP(), endMapperFunc, srcLocInfo, > > > MapTypesArg, > > > MapNamesArg, MapperAllocas, DeviceID, > > > MapTypeFlags.size()); > > > @@ -4100,11 +4104,9 @@ OpenMPIRBuilder::InsertPointTy > > > OpenMPIRBuilder::createTargetData( > > > MapTypeFlags.size()); > > > } > > > > > > - // Update the insertion point and remove the terminator we introduced. > > > - Builder.SetInsertPoint(UI->getParent()); > > > - if (IfCond) > > > - UI->getParent()->setName("omp_if.end"); > > > - UI->eraseFromParent(); > > > + if (ContBB) > > > + return {ContBB, ContBB->begin()}; > > > + > > > ``` > > I saw the other directives were making use of an explicit `ExitBB`, but for > > those directives they always needed to splice the directive into multiple > > BB's anyways. > > > > Since for target data we don't need to splice the BB unless using the `if` > > clause or `target region` are present, I was trying to find a way to do it > > without having to add a new BB at all times. IMO adding an unreachable inst > > that we always remove soon after is better than adding a new BB, but I'd > > defer to you in this case. > OK. You can retain the `unreachable` for now. Thanks. I've added a small comment in the code to explain what the `UI` is used for. ================ 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: > > > 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. 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