[llvm-branch-commits] [clang] [flang] [llvm] [mlir] [Flang][MLIR][OpenMP] Explicitly represent omp.target kernel types (PR #186166)

[Sergio Afonso via llvm-branch-commits]

https://github.com/skatrak commented:

Thank you Michael again for your comments.

> Have you considered keeping the old code and having `kernel_type` be an optional attribute, and only when it is missing use the automatic detection (and possibly in debug builds check whether it matches)? Even if Flang implementes its own logic, we may not want to force every other frontend to do the same.

Yes, I think this gets at the core of the issue here.

The idea with this is to start the process of removing completely any code path that currently relies on `TargetOp::getInnermostCapturedOmpOp()` and in fact force every frontend to tag things properly. In my opinion, I don't think we should use the existing logic at all because it's proved to be flaky and to be built on insufficient information. It's based on the incorrect assumption that the MLIR representation of an OpenMP combined construct, or that of two nested constructs with no additional statements, can be identified by checking that any sibling operations to the inner one are either pure or they have no memory write effects.

For instance, I recently ran into an issue where the initialization of local copies of array descriptors using `llvm.intr.memcpy` were identified as breaking the SPMD pattern, and had to relax the sibling checks to exclude writes to default resources and only check for the `AutomaticAllocationScopeResource`. This is not right, but I don't think there's any way right now to tell cases like that apart from a user introducing a statement to set some variable.

My thinking is that, if we introduced an `omp.combined` attribute similar to the existing `omp.composite` and got frontends to set it, together with the changes in this patch, we'd be able to reliably find the captured op by leveraging these attributes rather than some limited MLIR pattern matching.

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