[Mlir-commits] [llvm] [mlir] [mlir][openmp] - Fix crash in OpenMPIRBuilder when converting to LLVMIR (PR #84611)

[llvmlistbot at llvm.org]

agozillon wrote:

> My preference would be to still merge it, since it does fix some problems. We just need to be aware of its limitations and start thinking of a better way to deal with the root cause.
> 
> I think @agozillon and @jsjodin are more familiar with the reasons for the early outlining to be removed, but I seem to remember that it may have been done too early, resulting in complications when dealing with map clauses. I might be misremembering, though, so they might be able to correct me on that.

>From my recollection, as the early outlining was only being applied for the device, we still had (at least one of) the issues that it was originally there to prevent on the host pass, which was optimisation passes performing "malicious" (to TargetOp, debate-ably) movements of data out of the target region that caused issues in later lowering. This may or may not be addressable by another pass that occurs later after all optimisations have been ran that will re-affirm/canonicalize the map info fixing things that have been moved in and out of the region. The `IsolatedFromAbove` changes that we moved to addressed this as well though, so perhaps both approaches are not mutually exclusive.

The other issue that was more of a maintainability related issue that we encountered is that when utilised as a pass at HLFIR/FIR level it's very prone to requiring changes based on alterations to the HLFIR/FIR dialect as well as the OpenMP dialect as map_info (and likely other operations dependent on information that can be tied to Operations rather than ArgBlocks, i.e. anything that relies on attributes, or things like FIR/HLFIR address_of/declare which point to things like globals, bounds data etc.) and there associated captures variables would need cloned over into the newly generated outlined function and re-linked to the TargetOp. This might be alleviated to some extent by having the pass as something that occurs at an LLVM dialect level where the operations are simpler (at least from a map type perspective where everything is a pointer), but I imagine it'd still be prone to breakage from changes to either dialect, which is perhaps just something we'd have to live with. My primary recollection is having to change this pass a lot whenever implementing something new related to map clauses, however, this was early stages and might be something that would stop being an issue when we have the appropriate level of coverage!

I could go either way with an re-introduction of the EarlyOutliningPass, I am sure it has its merits as well as it's detriments to other methods we may introduce. 
 
I think @jsjodin had some other concerns as well that I cannot recollect off the top of my head unfortunately.

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