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

[Pranav Bhandarkar via llvm-commits]

bhandarkar-pranav wrote:

> Thanks for looking into this Pranav. It looks related to this downstream patch by @DominikAdamski: [ROCm#40](https://github.com/ROCm/llvm-project/pull/40).
> 
> It's yet another manifestation of the issues we have due to having to keep host code when compiling for the device just so that we're able to lower target regions inside. When we had the early outlining pass we didn't have this problem, because only device code remained by the time we lowered to LLVM IR. Now we're lowering all of the host code that exists in functions with an `omp.target` inside to then remove it later, which causes problems like what you're fixing here.
> 
> I think the long term solution for this is to actually remove host code from device compilation early (re-introducing some sort of early outlining, or at least somehow removing host operations in a pass while keeping target regions untouched). It seems difficult to selectively detect and skip lowering for these operations in advance in all cases, like Dominik's PR tries to do. And fixing the IR after lowering, like this patch does, is very difficult to get right for all situations, since lowering host operations might result in the creation of some OMPIRBuilder state used after the function has been deleted and this will change over time.
> 
> So I think this PR looks good as a short term solution to the issue, but we need to figure out a better plan long term. I'd like to hear from others' opinions about this more general issue. I don't oppose merging this PR, but I'll leave the approval to someone else.

Thanks for pointing me to https://github.com/ROCm/llvm-project/pull/40 @skatrak 
I agree this fix is very awkward and one can never be sure that all the ``OMPIRBuilder`` state has been cleaned up properly. The best is to avoid creating unnecessary state in the first place. I am curious about why early outlining was removed in the first place because on the surface early outlining makes sense to me (There must have been a good reason). In the meantime, I'll follow the discussion on @DominikAdamski's PR.

Do you suggest, I close this PR then? I need to fix this problem so I can continue working on lowering the `depend` clause. The lowering of the `depend` clause goes through a transformation (https://github.com/llvm/llvm-project/pull/83966) that converts
```
omp.target depend(...)
```
to 
```
omp.task depend(...) {
 omp.target {
 }
}
```
So, we will end up introducing host code anyway even in a function that has only the ``omp.target`` in it



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