[PATCH] D146557: [MLIR][OpenMP] Refactoring createTargetData in OMPIRBuilder

[Akash Banerjee via Phabricator via cfe-commits]

TIFitis added inline comments.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:4727
+  Builder.ClearInsertionPoint();
+}
+
----------------
jdoerfert wrote:
> This function doesn't make sense to me. For one, I don't know what a "unreal block" is. Nor would I have expected a block with terminator to be silently not touched. It might just be a documentation issue (in the header). I would avoid duplicating the comment here again.
I've removed the comment here and updated it in the header file.

The current comment was taken directly from Clang. I've updated it say exactly what it does.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:4785
+  EmitBlock(ContBlock, CurFn, /*IsFinished=*/true);
+}
+
----------------
jdoerfert wrote:
> CFG Utils have helpers for these things. Do we not use them on purpose?
If you are pointing at the `SplitBlockAndInsertIfThen` like functions in `llvm/lib/Transforms/Utils/BasicBlockUtils.cpp` then they require the current BB to be well formed with a terminator which it is not in this case.

Earlier we were adding an Unreachable inst and removing it later, however this function I think does a much cleaner job of handling all the cases including region code gen.

Also, it is not apparent in this patch but future patches including some from Jan in review make use of the emitBlock and emitBranch functions directly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146557