[Openmp-commits] [PATCH] D138495: [openmp][mlir] Lower parallel if to new fork_call_if function.

[Kiran Chandramohan via Phabricator via Openmp-commits]

kiranchandramohan added a comment.

Looks fine to me.

There are clang-format issues.

  changed files:
      llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
      openmp/runtime/src/kmp.h



================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:967
 
   // ThenBB
   //   |
----------------
Does this need an update since there is no `ThenBB`?


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1030
 
     // Build call __kmpc_fork_call(Ident, n, microtask, var1, .., varn);
     Value *ForkCallArgs[] = {
----------------
Nit: This comment needs an update.


================
Comment at: mlir/test/Target/LLVMIR/openmp-llvm.mlir:197
-// CHECK-LABEL: @test_nested_alloca_ip
-llvm.func @test_nested_alloca_ip(%arg0: i32) -> () {
-
----------------
Was there any particular reason to remove this test? Can you update this test instead?


================
Comment at: openmp/runtime/src/kmp_csupport.cpp:344
+void __kmpc_fork_call_if(ident_t *loc, kmp_int32 argc, kmpc_micro microtask,
+                         bool cond, void *args) {
+  int gtid = __kmp_entry_gtid();
----------------
Would we ever want a variable length parameter option like the `kmpc_fork_call`?


================
Comment at: openmp/runtime/src/kmp_csupport.cpp:347
+  int zero = 0;
+  if (cond)
+    __kmpc_fork_call(loc, argc, microtask, args);
----------------
Nit: Use braces here to match the else.


================
Comment at: openmp/runtime/src/kmp_csupport.cpp:351
+    __kmpc_serialized_parallel(loc, gtid);
+    microtask(&gtid, &zero, args);
+    __kmpc_end_serialized_parallel(loc, gtid);
----------------
Would this launch a separate thread?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138495