[PATCH] D71267: [OpenMPIRBuilder] Add support for generating kmpc_for_static_fini

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added inline comments.


================
Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:2819
+  }
 }
 
----------------
kiranchandramohan wrote:
> jdoerfert wrote:
> > We do emit exactly the same code in both cases, don't we?
> > If so, use the always available OMPIRBuilder unconditionally.
> As of now there is a minor difference. The IRBuilder static fini generates a new call to kmpc_global_thread_num, the existing flow reuses the value computed by a previous call to kmpc_global_thread_num. Seems to be the same functionality.
> ```
> <   call void @__kmpc_for_static_fini(%struct.ident_t* @1, i32 %8)
> ---
> >   %31 = call i32 @__kmpc_global_thread_num(%struct.ident_t* @2)
> >   call void @__kmpc_for_static_fini(%struct.ident_t* @1, i32 %31)
> ```
ok, do you think you can update the tests here by hand? That would allow to remove the old handling right away.



================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPConstants.h:92
+bool isOpenMPLoopDirective(Directive DKind);
+
 } // end namespace omp
----------------
kiranchandramohan wrote:
> jdoerfert wrote:
> > Where is the definition for these?
> Good catch. These are in the OMPConstants.cpp file. But i guess we do not want that file.
> 
> So we can generate these from the tablegen and remove their declaration here and in OMPConstants.cpp. Is that the right approach?
I think the tablegen way is good *if* we can make it nice. If not, we can keep the constants.cpp.
Nice would be, for example, if we generate the `isOpenMPNestingDistributeDirective` by checking if the directive name starts with `distribute`. `isOpenMPDistributeDirective` is similar, `isOpenMPLoopDirective` will probably require a flag in the directives, though that seems ok and useful either way.

WDYT?


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

https://reviews.llvm.org/D71267