[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert marked an inline comment as done.
jdoerfert added a comment.

In D69785#1762438 <https://reviews.llvm.org/D69785#1762438>, @JonChesterfield wrote:

> I'd very much like this to land soon. It's the prereq for a lot of other patches and the code looks good.
>
> It's tricky to test the infra before the users are landed so the unit test is particularly appreciated.


I'm confused. Was this a review? I'm waiting for a decision here so we can move on and improve on this instead of me modifying it inp-lace two comments at a time.



================
Comment at: llvm/include/llvm/Frontend/OpenMPKinds.def:165
+
+__OMP_RTL(__kmpc_barrier, false, Void, IdentPtr, Int32)
+__OMP_RTL(__kmpc_cancel_barrier, false, Int32, IdentPtr, Int32)
----------------
rogfer01 wrote:
> As we migrate, we will end with a significant number of interfaces here.
> 
> @jdoerfert what do you think about adding a comment with their C prototype before each one like we do in `clang/lib/CodeGen/CGOpenMPRuntime.cpp`?
> 
> Something like this
> 
> ```lang=cpp
> // void __kmpc_barrier(ident_t *loc, kmp_int32 global_tid);
> __OMP_RTL(__kmpc_barrier, false, Void, IdentPtr, Int32)
> // kmp_int32 __kmpc_cancel_barrier(ident_t *loc, kmp_int32
> // global_tid)
> __OMP_RTL(__kmpc_cancel_barrier, false, Int32, IdentPtr, Int32)
> ...
> ```
I'm fine with this but I doubt it'll help much (compared to the lines we have that show name and types).

If you want this to happen you should create a patch do add comments for the ones we have here, and others can way in. If there is agreement to apply it, we will do so and continue that tradition from then on. Does that sound good?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69785