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

[Kiran Chandramohan via Phabricator via llvm-commits]

kiranchandramohan requested changes to this revision.
kiranchandramohan added a comment.
This revision now requires changes to proceed.

I have some more comments.



================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:1556
 
+  /// Generator for '#omp target data'
+  ///
----------------
Nit: In the following comments either use full stops for all.

It will be helpful if the comments below refer to the OpenMP construct or clauses rather than the MLIR representation. Forr e.g Map Clause instead of Map operand.


================
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
----------------
Can't the presence/absence of the BodyGenCB indicate the presence/absence of a region?


================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:1567
+  /// \param MapperAllocas Pointers to the AllocInsts for the map clause
+  /// \param MapperFunc Mapper Fucntion to be called for the Target Data op
+  /// \param DeviceID Stores the DeviceID from the device clause
----------------
This should be expanded to say whether it means `begin` or `end` mapper based on the value of the boolean and probably also renamed to `isBegin` or `isEnter` or something like that.


================
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:
> > 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()};
+
```


================
Comment at: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp:4915
 
+TEST_F(OpenMPIRBuilderTest, TargetEnterData) {
+  OpenMPIRBuilder OMPBuilder(*M);
----------------
In general, we have to test more in these unit tests. At the moment this only tests the `enter data` variant.


================
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,
----------------
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?


================
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;
----------------
Can all this go into the OpenMP IRBuilder? And be passed back if necessary via the callback.


================
Comment at: mlir/test/Target/LLVMIR/omptarget-llvm.mlir:49-80
+    %4 = llvm.mlir.constant(1 : i32) : i32
+    %5 = llvm.sext %4 : i32 to i64
+    %6 = llvm.mlir.constant(1024 : i32) : i32
+    %7 = llvm.sext %6 : i32 to i64
+    %8 = llvm.mlir.constant(1 : index) : i64
+    %9 = llvm.trunc %5 : i64 to i32
+    %10 = llvm.sub %7, %5  : i64
----------------
Nit: Reduce this region to the minimum. One or two instructions in the region is sufficient.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D142914/new/

https://reviews.llvm.org/D142914