[Openmp-commits] [PATCH] D95976: [OpenMP] Simplify offloading parallel call codegen

[Johannes Doerfert via Phabricator via Openmp-commits]

jdoerfert added a comment.

I have only minor remarks but I'd like you to check if my hunch is correct and the proposed modifications will fix fix PR49777 *and* fix PR49779.
Also, the number of arguments need to be increased, let's go big and automatic here.

Other than that I think this looks good.



================
Comment at: clang/lib/CodeGen/CGOpenMPRuntimeGPU.cpp:2192
     RCG(CGF);
   }
 }
----------------
Can we remove SeqGen while we are here please. We need to check in the runtime anyway. That check is later folded, no need to make things more complicated here.


================
Comment at: openmp/libomptarget/deviceRTLs/common/src/parallel.cu:294
+  // TODO: Add UNLIKELY to optimize?
+  if (!if_expr) {
+    __kmpc_serialized_parallel(ident, global_tid);
----------------
This should allow us to remove the `SeqGen` in the Clang CodeGen *and* fix PR49777 *and* fix PR49779, a win-win-win situation.


================
Comment at: openmp/libomptarget/deviceRTLs/common/src/parallel.cu:368
+  //  __kmpc_push_proc_bind(ident, global_tid, proc_bind);
+}
+
----------------
FWIW, The implementation here is a stopgap until we move to the new runtime. The codegen and interface are the important parts.


================
Comment at: openmp/libomptarget/deviceRTLs/common/src/support.cu:370
+    printf("Too many arguments in kmp_invoke_microtask, aborting execution.\n");
+    return;
+  }
----------------
Not a return but a `__builtin_trap()`, please.
We also need this for more than 16 unfortunately, I've seen 20 in miniqmc.
We might want to create a script to print the cases, and then generate 128 or something like that in a file we include. The script can be in the utils folder too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95976