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

Thu May 19 08:21:43 PDT 2022

Meinersbur accepted this revision.
Meinersbur added a comment.
This revision is now accepted and ready to land.

LGTM. Consider adding some description of TaskSize before committing.

================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1300
+    Value *TaskSize = Builder.getInt64(0);
+    if (HasTaskData) {
+      AllocaInst *ArgStructAlloca =
----------------
shraiysh wrote:
> 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.
https://github.com/llvm/llvm-project/blob/a4190037fac06c2b0cc71b8bb90de9e6b570ebb5/openmp/runtime/src/kmp_tasking.cpp#L1345
```
// sizeof_kmp_task_t:  Size in bytes of kmp_task_t data structure including
// private vars accessed in task.
```

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71989