[PATCH] D142914: [MLIR][OpenMP] Added OMPIRBuilder support for Target Data directives.

[Akash Banerjee via Phabricator via cfe-commits]

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