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

[Akash Banerjee via Phabricator via llvm-commits]

TIFitis added inline comments.


================
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:
> 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142914