[Mlir-commits] [mlir] [WIP][OpenMP][MLIR] Lowering task_reduction clause for pass-by-value vars to LLVMIR (PR #125218)

[Tom Eccles]

tblah wrote:

Hi, thanks for coming back to this

>     1. Is it better if this functionality is offloaded to the OMPIRBuilder, or is the current setup okay?
I think the original idea was that OMPIRBuilder would allow clang and flang to share OpenMP codegen. That design has gained little traction and so far as I know work to get clang to use OMPIRBuilder by default has stalled. I think we already implement some things outside OMPIRBuilder where there was no existing support in OMPIRBuilder, so feel free to go ahead with the current set up.

I think sharing code between flang and clang was a nice idea but in practice I find all of the callbacks and delayed function outlining in OMPIRBuilder a bit difficult to understand, whereas some of the pure mlir implementations (bypassing OMPIRBuilder) added recently are much easier to follow (at least for me).

>     2. You mentioned that [[flang][mlir] Add support for translating task_reduction to LLVMIR #120957](https://github.com/llvm/llvm-project/pull/120957) assumes that all SSA values passed into the reduction are stack allocated, which is not always true (lines 1965 to 1973 in OpenMPToLLVMIRTranslation.cpp in that PR). Could you point me to a relevant PR or some similar functionality which can help me resolve this issue? I'm a bit clueless about this.
For a quick solution to get this PR moving I would recommend looking for an equivalent for non-task reductions. Hopefully we can get around needing to know this allocation size entirely.

Otherwise, we might need something like my recent changes to omp.private (https://discourse.llvm.org/c/subprojects/flang/33) so that the allocation becomes implicit in omp.declare_reduction and the type to be allocated can be read straight from the operation definition. In an ideal world I would like some day to merge `omp.private` and `omp.delcare_reduction` into one operation (the reduction combiner region could be an optional region similar to a firstprivate copy region). But I cannot currently commit to finding the time to do this myself (although I will try to make some time).

But I appreciate that the above solution would be quite a bit of work, so I think if you cannot find a way to work around this, it would be appropriate to make an RFC or discuss this in an openmp call to see how others feel about adding this assumption about stack allocation to omp.declare_reduction. If everyone is happy it might be good enough to just document the assumption and perhaps make the verifier for the alloc region stricter.

>     3. This version of the LLVMIR translation emits "empty" function bodies for init and combiner (instead of nullptr as [[flang][mlir] Add support for translating task_reduction to LLVMIR #120957](https://github.com/llvm/llvm-project/pull/120957) was doing) in case we encounter an empty region for these functions. For finalization is optional, so a nullptr is emitted. Hope this is okay. Or should we throw an assertion here?

That sounds good to me. It is okay to assume that the reduction operation passes the verifier (all non-optional regions are present).

Also, about my comment on the test worrying about pointer lifetimes: I looked into that just now and it looks like the openmp runtime automatically copies out of this data into heap allocated memory when creating the task, so this shouldn't be a problem. See https://github.com/llvm/llvm-project/blob/eb0af4e48d0e039849c6bbf36e791610e7ef9a06/openmp/runtime/src/kmp_tasking.cpp#L2525

I hope all that makes sense. Please feel free to reach out here or on slack otherwise :smile: 



https://github.com/llvm/llvm-project/pull/125218