[Mlir-commits] [mlir] [mlir][SPIRV] Fix lookup logic `spirv.target_env` for `gpu.module` (PR #147262)

[Jaeho Kim]

oojahooo wrote:

> > If target env is inserted via the `spirv-attach-target` pass when no op—including `builtin.module` or `gpu.module`—has an existing one, then under the current implementation, the target env is attached only to the `"targets"` array attribute of `gpu.module`, and does not appear elsewhere.
> > When the `convert-gpu-to-spirv` pass is run in this case, it fails to find the target env in `"targets"` and instead falls back to the default target env, which leads to a legalization failure of the `memref.load` op.
> 
> To me the sounds like the wrong fix. Could we teach the passes you mentioned to also look in `"targets"` instead of relying on the core `lookupTargetEnv` helper that doesn't know anything about the `gpu` dialect?

If I understand your point correctly, rather than hardcoding the `lookupTargetEnv` helper to directly search for the `"targets"` attribute, which is specific to the `gpu` dialect, it would be more appropriate to handle this logic within the `convert-gpu-to-spirv` pass.

There are two possible approaches that I think:

1. As you suggested, we could modify the `convert-gpu-to-spirv` pass to explicitly inspect the `"targets"` attribute of `gpu.module` and retrieve the corresponding `spirv.target_env`. If that lookup fails, we can fall back to invoking the `spirv::lookupTargetEnvOrDefault` function. I believe this is a reasonable solution since the pass is already coupled with the `gpu` dialect by design.

2. Alternatively, we could generalize the logic of `lookupTargetEnv` to retrieve all attributes via `getAttrDictionary()` and recursively walk through them to locate a `spirv.target_env` attribute. While this approach differs slightly from your suggestion, I thought it could more robustly support the intended semantics of passes using `lookupTargetEnvOrDefault`, and would also remain dialect-agnostic.

I would greatly appreciate it if you could confirm whether this interpretation is aligned with your suggestion. If so, I would be happy to revise the implementation accordingly based on the preferred direction.

Thank you for your support.

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