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

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added inline comments.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1232-1242
+  // Basic block mapping for task generation:
+  // ```
+  // current_fn() {
+  //   currbb
+  //   task.exit
+  // }
+  // outlined_fn() {
----------------
I don't understand what this means.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1243
+  // ```
+  BasicBlock *currbb = Builder.GetInsertBlock();
+  BasicBlock *TaskAllocaBB = currbb->splitBasicBlock(UI, "task.alloca");
----------------
[style] Please use [[ https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly | LLVM's naming standard ]].


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1255
+  OI.EntryBB = TaskAllocaBB;
+  OI.OuterAllocaBB = AllocaIP.getBlock();
+  OI.ExitBB = TaskExitBB;
----------------
`BodyGenCB` may have invalidated `AllocaIP` and cannot be used anymore.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1300
+    Value *TaskSize = Builder.getInt64(0);
+    if (HasTaskData) {
+      AllocaInst *ArgStructAlloca =
----------------
Please document what `HasTaskData`. `TaskSize`, `NewTaskData`, etc are. 


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1320
+      WrapperArgTys.push_back(OutlinedFn.getArg(0)->getType());
+    std::string WrapperFuncName = OutlinedFn.getName().str() + ".wrapper";
+    FunctionCallee WrapperFuncVal = M.getOrInsertFunction(
----------------
Use a `llvm::Twine`


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1335
+        TaskAllocFn,
+        {Ident, ThreadID, /*flags=*/Builder.getInt32(1),
+         /*sizeof_task=*/TaskSize, /*sizeof_shared=*/Builder.getInt64(0),
----------------
What is flag `1`?


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1354-1355
+    // Emit the body for wrapper function
+    BasicBlock *WrapperEntryBB = BasicBlock::Create(M.getContext());
+    WrapperFunc->getBasicBlockList().push_back(WrapperEntryBB);
+    Builder.SetInsertPoint(WrapperEntryBB);
----------------



================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1366
+  Builder.SetInsertPoint(UI->getParent());
+  UI->eraseFromParent();
+
----------------
Please avoid temporary instructions. Use `splitBB` instead.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1109-1114
+    // func @current_fn() {
+    //   runtime_call(..., wrapper_fn, ...)
+    // }
+    // func @wrapper_fn(..., %args) {
+    //   outlined_fn(%args)
+    // }
----------------
shraiysh wrote:
> 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.
Could you add the function signature differences between `wrapper_fn` and `outlined_fn`? I.e. what arguments does `wrapper_fn` throw away?

Btw, current Clang emits such a wrapper function with `-g` as well, so I don't see it as an issue. LLVM will always inline a private function with just a single call site, except in `-O0` where `always_inline` would be needed. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71989