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

[Shraiysh via Phabricator via Openmp-commits]

shraiysh 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() {
----------------
Meinersbur wrote:
> 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.
> `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.
Yes, that was supposed to justify why we need four splits here. Without this comment it wasn't really clear to me why three blocks won't be enough (currBB, task.body and task.exit). It also isn't directly intuitive that task.exit is not going to be a part of the outlined function. I have added that this is going to be the basic block mapping after outlining. If it seems unnecessary, please let me know and I will remove this comment.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1300
+    Value *TaskSize = Builder.getInt64(0);
+    if (HasTaskData) {
+      AllocaInst *ArgStructAlloca =
----------------
Meinersbur wrote:
> 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?
I have added that TaskSize refers to the argument `sizeof_kmp_task_t` in the [[ https://github.com/llvm/llvm-project/blob/a4190037fac06c2b0cc71b8bb90de9e6b570ebb5/openmp/runtime/src/kmp.h#L3774 | runtime call ]]. I did not want to add further information because the argument's exact meaning is not documented anywhere (the reference doesn't have it). My interpretation of the argument is the size of arguments in bytes, but that could be incorrect and I thought it was better to redirect anyone reading this/working on this to the argument of the runtime call instead of writing a possibly misleading description. Please let me know if it would be better to add the current interpretation of the argument.

> For HasTaskData, could you explain why and how the function signature changes?
Documented this near the wrapper function. Please let me know if it requires some alteration.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71989