[PATCH] D69922: [OpenMP] Use the OpenMP-IR-Builder

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert marked 3 inline comments as done.
jdoerfert added a comment.

In D69922#1742392 <https://reviews.llvm.org/D69922#1742392>, @ABataev wrote:

> In D69922#1741659 <https://reviews.llvm.org/D69922#1741659>, @jdoerfert wrote:
>
> > Use IRBuilder for cancel barriers
>
>
> In general, it would better to implement cancellation barriers in the separate patch.  Same for OpenMPIrBuilder.


Why would you think that splitting these closely related concepts would be beneficial?

In D69922#1751827 <https://reviews.llvm.org/D69922#1751827>, @kiranchandramohan wrote:

> Thanks @jdoerfert for working on this.
>
> Sorry for being late to the party. I am trying to implement one trivial directive (Flush) and one slightly more involved (not decided).
>
> I applied D69853 <https://reviews.llvm.org/D69853>, D69785 <https://reviews.llvm.org/D69785>, D69922 <https://reviews.llvm.org/D69922> to my local build and found that D69922 <https://reviews.llvm.org/D69922> is referring to OpenMPIRBuilder.h in llvm/Frontend whereas in D69785 <https://reviews.llvm.org/D69785> it was introduced in llvm/Transforms/Utils/OpenMPIRBuilder.h


The conversation (on the llvm-dev list) seems to have settled on `{include/llvm,lib}/frontend/OpenMP/`. I will move the files around once we finally settle these reviews that haven't moved (more than 2 comments at a time).
As you can see, I have a huge backlog here and that is really hard to modify. You have a problem now as you depend on these. @hfinkel mentioned it in his email to the llvm-dev (about review etiquette), we need to
minimize iterations in reviews and actually review patches not only a few lines every time.



================
Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:3492
+          CGF.createBasicBlock(".cancel.exit", IP.getBlock()->getParent());
+      OMPBuilder->setCancellationBlock(ExitBB);
+      CGF.Builder.SetInsertPoint(ExitBB);
----------------
ABataev wrote:
> Maybe, instead of saving the state, pass the pointer to the cancel block as a parameter to the `CreateBarrier` function?
All of this goes away in D70258, as it should. The frontend when creating a barrier does not need to know if it is cancelable and if so, where the cancel block is.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69922