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

[Shraiysh via Phabricator via llvm-commits]

shraiysh added inline 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-
----------------
kiranchandramohan wrote:
> 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`.
I have not added the attributes because I don't know about them and if they are required or not. I will check clang codegen once again for comments about any required attributes, but if you or anyone else knows what attributes are required and why, please let me know, I will add them.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1109-1114
+    // func @current_fn() {
+    //   runtime_call(..., wrapper_fn, ...)
+    // }
+    // func @wrapper_fn(..., %args) {
+    //   outlined_fn(%args)
+    // }
----------------
kiranchandramohan wrote:
> 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?
The main reason for a difference is that Outlining generates a function call as shown in the input IR in the comment above. I could not find an easy way to change the arguments of a function after it has been constructed. Parallel does this by tweaking the CodeExtractor used inside outlining - and I did not want to touch outlining internally so that I can confidently rely on outlining to do its job.

A nice way to have similar (but not same) behavior as clang would be to add the inline attribute to the outlined function. After an Inline pass, the wrapper function will become the outlined function and it will be almost identical to clang's codegen. I will update the summary to highlight this difference.


================
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});
----------------
kiranchandramohan wrote:
> 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.
Yes, that is a todo, I have added it now. I think the task size will be reduced in that case.  I have not looked into that yet and am doing private copies for everything at the moment.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1198
+    Builder.CreateRet(Builder.getInt32(0));
+  };
+
----------------
Meinersbur wrote:
> kiranchandramohan wrote:
> > Nit: A debug dump of the IR here might be useful.
> Please don't dump complete IR. Too much output makes `-debug` useless.
Please let me know if something less than IR should be dumped here.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1202
+  SmallVector<BasicBlock *, 32> Blocks;
+  OI.collectBlocks(TaskRegionBlockSet, Blocks);
+
----------------
kiranchandramohan wrote:
> 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.
No it is not, I thought `collectBlocks` was required. I have removed it now.


================
Comment at: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp:1197-1198
   EXPECT_TRUE(Arg2Type->isPointerTy());
-  EXPECT_TRUE(cast<PointerType>(Arg2Type)
-                  ->isOpaqueOrPointeeTypeMatches(ArgStructTy));
+  EXPECT_TRUE(
+      cast<PointerType>(Arg2Type)->isOpaqueOrPointeeTypeMatches(ArgStructTy));
 }
----------------
Meinersbur wrote:
> [nit] only whitespace change
Removed. Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71989