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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits