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:
> 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`?
================
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:
> > > 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.
================
Comment at: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp:4915
+TEST_F(OpenMPIRBuilderTest, TargetEnterData) {
+ OpenMPIRBuilder OMPBuilder(*M);
----------------
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?
================
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:
> 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?
================
Comment at:
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:1502-1510
+ llvm::OpenMPIRBuilder::LocationDescription ompLoc(builder);
+ llvm::OpenMPIRBuilder::InsertPointTy allocaIP =
+ findAllocaInsertPoint(builder, moduleTranslation);
+
+ struct llvm::OpenMPIRBuilder::MapperAllocas mapperAllocas;
+ SmallVector<uint64_t> mapTypeFlags;
+ SmallVector<llvm::Constant *> mapNames;
----------------
kiranchandramohan wrote:
> Can all this go into the OpenMP IRBuilder? And be passed back if necessary
> via the callback.
If we decide to move processMapOperand then these can be moved along with it.
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
