[PATCH] D71989: [OpenMP][IRBuilder] `omp task` support

[Kiran Chandramohan via Phabricator via llvm-commits]

kiranchandramohan added a comment.

Thanks @shraiysh for taking up the work for task. This is the most important pending work for non-target OpenMP.

Could you expand the Summary a bit more with the approach taken and how it differs from current Clang task codegen?

I have added a few questions and comments.



================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1097
+  OI.ExitBB = TaskExitBB;
+  OI.PostOutlineCB = [this](Function &OutlinedFn) {
+    // The input IR here looks like the following-
----------------
Do you know whether attributes are added to the outlined/wrapper function? Is it added in the finalize function? `CreateParallel` seems to have in in the `PostOutlineCB`.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1109-1114
+    // func @current_fn() {
+    //   runtime_call(..., wrapper_fn, ...)
+    // }
+    // func @wrapper_fn(..., %args) {
+    //   outlined_fn(%args)
+    // }
----------------
Is this a change from clang codegen? Why not runtime_call(..., outlined_fn, ...)? Can you say that explicitly in the summary that this is different from Clang if it was changed for a particular reason?


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1142
+      assert(ArgStructAlloca &&
+             "Unable to find the alloca instruction coressponding to arguments "
+             "for extracted function");
----------------
Nit: corresponding


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1172
+        {Ident, ThreadID, /*flags=*/Builder.getInt32(1),
+         /*sizeof_task=*/TaskSize, /*sizeof_shared=*/Builder.getInt64(0),
+         /*task_func=*/WrapperFuncBitcast});
----------------
Is the size of shared a TODO? And would further changes impact the task size as well?
It seems by default variables are passed as private copies, but if the enclosing region has a data-sharing of shared then that should be honoured.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1198
+    Builder.CreateRet(Builder.getInt32(0));
+  };
+
----------------
Nit: A debug dump of the IR here might be useful.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1200-1201
+
+  SmallPtrSet<BasicBlock *, 32> TaskRegionBlockSet;
+  SmallVector<BasicBlock *, 32> Blocks;
+  OI.collectBlocks(TaskRegionBlockSet, Blocks);
----------------
Nit: The size can be omitted.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1202
+  SmallVector<BasicBlock *, 32> Blocks;
+  OI.collectBlocks(TaskRegionBlockSet, Blocks);
+
----------------
Is this step required? `TaskRegionBlockSet` and `Blocks` do not seem to be used here, also `collectBlocks` function seems to be called from finalize as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71989