[PATCH] D142914: [MLIR][OpenMP] Added OMPIRBuilder support for Target Data directives.
Kiran Chandramohan via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Tue Mar 7 02:36:09 PST 2023
kiranchandramohan added a comment.
Thanks for the update and the replies. See comments inline.
================
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
----------------
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 = {});
```
================
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");
----------------
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.
================
Comment at: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp:4915
+TEST_F(OpenMPIRBuilderTest, TargetEnterData) {
+ OpenMPIRBuilder OMPBuilder(*M);
----------------
TIFitis wrote:
> kiranchandramohan wrote:
> > In general, we have to test more in these unit tests. At the moment this only tests the `enter data` variant.
> The `IRBuilder` part of the code doesn't really generate much code in itself. It mainly calls the `BodyGenCB` and `ProcessMapCB`, and makes use of the existing `emitMapperCall` function.
>
> Test for `emitMapperCall` is already present in the unit test and the `CB` functors are tested through the MLIR test. Thus I've only added a single unit test for the `IRBuilder`, would you like me to add a few more?
Yes, please add a test with a region and for exit data as well.
================
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:
> > 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.
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