[PATCH] D80222: Replace Clang's createRuntimeFunction with the definitions in OMPKinds.def

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.

> libomp used to pass until I changed it to us getOrCreateRuntimeFunction I think.

Those tests are flaky, I think. Just run them a few times. I don't expect this to influence them at all.



================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:203
+  static Function *getOrCreateRuntimeFunction(Module &Md,
+                                              omp::RuntimeFunction FnID);
 
----------------
jhuber6 wrote:
> jdoerfert wrote:
> > Nit: M is the commonly used name for a module ;)
> The OpenMPIRBuilder struct has a Module named M so I wasn't sure if I should be shadowing it.
Go with `M`, it's static :)


================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPKinds.def:483
+__OMP_RTL(__kmpc_push_num_teams, false, /* Void? */ Int32, IdentPtr, Int32, 
+          Int32, Int32)
 
----------------
jhuber6 wrote:
> For this one there's conflicting information on the return value. Clang expects this in its checks
> // CK1: {{%.+}} = call i32 @__kmpc_push_num_teams({{.+}}, {{.+}}, i32 [[TE_VAL]], i32 [[TH_VAL]])
> 
> But when I look at the runtime I find this 
> void __kmpc_push_num_teams(ident_t *loc, kmp_int32 global_tid,
>                            kmp_int32 num_teams, kmp_int32 num_threads) {
> 
Make it void here please, will need to update the tests:
`sed -i -e 's:i32 @__kmpc_push_num_teams:void @__kmpc_push_num_teams:' clang/test/OpenMP/*.*`


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:103
+      }
+    } else if (Fn->getName() == "__kmpc_fork_teams") {
+      if (!Fn->hasMetadata(LLVMContext::MD_callback)) {
----------------
jhuber6 wrote:
> jdoerfert wrote:
> > Please use `FnID` for the comparison.
> Don't know why I didn't think of doing that, will do.
And if the callback is the same for both, merge the conditionals.


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

https://reviews.llvm.org/D80222