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

Sat Apr 30 06:29:36 PDT 2022

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:
> 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?

================
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(
----------------
Meinersbur wrote:
> Use a `llvm::Twine`
I have used it, but I am not sure if it is accurate usage. Please let me know if it seems inefficient.

================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1335
+        TaskAllocFn,
+        {Ident, ThreadID, /*flags=*/Builder.getInt32(1),
+         /*sizeof_task=*/TaskSize, /*sizeof_shared=*/Builder.getInt64(0),
----------------
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.

================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1366
+  Builder.SetInsertPoint(UI->getParent());
+  UI->eraseFromParent();
+
----------------
Meinersbur wrote:
> Please avoid temporary instructions. Use `splitBB` instead.
AFAIK, `splitBB` requires an instruction pointer. I have updated this to erase the temporary instruction immediately after I have split the basic blocks, but I cannot figure out a way to completely eliminate temporary instructions. Is this okay? 

================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1109-1114
+    // func @current_fn() {
+    //   runtime_call(..., wrapper_fn, ...)
+    // }
+    // func @wrapper_fn(..., %args) {
+    //   outlined_fn(%args)
+    // }
----------------
Meinersbur wrote:
> 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. 
The first argument is an i32 value but I do not know its usage. There is minimal documentation for task related stuff in OpenMP Runtime Library Reference in the OpenMP subproject. [[ https://github.com/llvm/llvm-project/blob/371412e065a63107d5d79330da6757ff693d91cc/openmp/runtime/src/kmp.h#L2286 | Here ]] is the function signature for the wrapper function, and the first argument is discarded at the moment. If anyone has any reference to some documentation about this, or has any idea about what that argument represents, please let me know and I will try to handle it according to its purpose.

> 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. 

Oh that is great then. Thank you!

================
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:
> Meinersbur wrote:
> > 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. 
> The first argument is an i32 value but I do not know its usage. There is minimal documentation for task related stuff in OpenMP Runtime Library Reference in the OpenMP subproject. [[ https://github.com/llvm/llvm-project/blob/371412e065a63107d5d79330da6757ff693d91cc/openmp/runtime/src/kmp.h#L2286 | Here ]] is the function signature for the wrapper function, and the first argument is discarded at the moment. If anyone has any reference to some documentation about this, or has any idea about what that argument represents, please let me know and I will try to handle it according to its purpose.
> 
> > 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. 
> 
> Oh that is great then. Thank you!
> 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