[PATCH] D142914: [MLIR][OpenMP] Added OMPIRBuilder support for Target Data directives.
Akash Banerjee via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Tue Mar 7 12:58:43 PST 2023
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
More information about the cfe-commits
mailing list