[Openmp-commits] [PATCH] D71989: [OpenMP][IRBuilder] `omp task` support

[Michael Kruse via Phabricator via Openmp-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() {
----------------
shraiysh wrote:
> Meinersbur wrote:
> > I don't understand what this means.
> There are three basic block splits in the next few lines and this comment is meant to justify why three splits are needed - it basically tells which basic block will go where. Should I add something more to the comment?
Consider something that resembles LLVM-IR syntax. Eg.

```
  // def outlined_fn() {
  //   task.alloca:
  //     br label %task.body
  //
  //   task.body:
  //     ret void
  // }
```
`outlined_fn` is a view after the finalize call, but here we are constructing the CFG before outlining, i.e. the description does not match.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1300
+    Value *TaskSize = Builder.getInt64(0);
+    if (HasTaskData) {
+      AllocaInst *ArgStructAlloca =
----------------
Meinersbur wrote:
> Please document what `HasTaskData`. `TaskSize`, `NewTaskData`, etc are. 
Still don't see a description of `TaskSize`.

For `HasTaskData`, could you explain why and how the function signature changes?


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1335
+        TaskAllocFn,
+        {Ident, ThreadID, /*flags=*/Builder.getInt32(1),
+         /*sizeof_task=*/TaskSize, /*sizeof_shared=*/Builder.getInt64(0),
----------------
shraiysh wrote:
> Meinersbur wrote:
> > What is flag `1`?
> This is from [[ https://github.com/llvm/llvm-project/blob/e1567e771b8943861f5c886773b19ebfbf395ac5/openmp/runtime/src/kmp.h#L2442 | flags in kmp.h ]]. Value 1 means that it is tied (which is default unless `untied` clause is specified). Untied clause is not handled in this patch.
Please document that.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1324-1326
+    Value *Ident = getOrCreateIdent(
+        getOrCreateSrcLocStr(LocationDescription(Builder), SrcLocStrSize),
+        SrcLocStrSize);
----------------
[serious] `SrcLocStrSize` cannot be passed-by-value and passed-bt-referenced at the same statement (unsequenced).


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1351-1352
+      WrapperArgTys.push_back(OutlinedFn.getArg(0)->getType());
+    llvm::Twine WrapperFuncName = OutlinedFn.getName() + ".wrapper";
+    SmallString<128> WrapperFuncNameStorage;
+    FunctionCallee WrapperFuncVal = M.getOrInsertFunction(
----------------
`WrapperFuncName` and `WrapperFuncNameStorage` are now dead. Could you remove them?


================
Comment at: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp:4538
+
+  auto *AllocaBB = Builder.GetInsertBlock();
+  auto *BodyBB = splitBB(Builder, /*CreateBranch=*/true, "alloca.split");
----------------
[style]


================
Comment at: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp:4539
+  auto *AllocaBB = Builder.GetInsertBlock();
+  auto *BodyBB = splitBB(Builder, /*CreateBranch=*/true, "alloca.split");
+  OpenMPIRBuilder::LocationDescription Loc(
----------------
[style]


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71989