[mlir][spirv] Fix lookup logic `spirv.target_env` for `gpu.module` #147262

oojahooo · 2025-07-07T09:32:39Z

The gpu.module operation can contain spirv.target_env attributes within an array attribute named "targets". So it accounts for that case by iterating over the "targets" attribute, if present, and looking up spirv.target_env.

The `gpu.module` operation can contain `spirv.target_env` attributes within an array attribute named `"targets"`. So it accounts for that case by iterating over the `"targets"` attribute, if present, and looking up `spirv.target_env`.

github-actions · 2025-07-07T09:32:58Z

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

llvmbot · 2025-07-07T09:33:28Z

@llvm/pr-subscribers-mlir-gpu

@llvm/pr-subscribers-mlir-spirv

Author: Jaeho Kim (oojahooo)

Changes

The gpu.module operation can contain spirv.target_env attributes within an array attribute named "targets". So it accounts for that case by iterating over the "targets" attribute, if present, and looking up spirv.target_env.

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

1 Files Affected:

(modified) mlir/lib/Dialect/SPIRV/IR/TargetAndABI.cpp (+9)

diff --git a/mlir/lib/Dialect/SPIRV/IR/TargetAndABI.cpp b/mlir/lib/Dialect/SPIRV/IR/TargetAndABI.cpp
index 5ecbd5d7c59d5..dbaa10e89bd42 100644
--- a/mlir/lib/Dialect/SPIRV/IR/TargetAndABI.cpp
+++ b/mlir/lib/Dialect/SPIRV/IR/TargetAndABI.cpp
@@ -184,6 +184,15 @@ spirv::TargetEnvAttr spirv::lookupTargetEnv(Operation *op) {
     if (!op)
       break;
 
+    if (auto arrAttr = op->getAttrOfType<ArrayAttr>("targets")) {
+      for (auto attr : arrAttr) {
+        if (auto spirvTargetEnvAttr =
+                llvm::dyn_cast<spirv::TargetEnvAttr>(attr)) {
+          return spirvTargetEnvAttr;
+        }
+      }
+    }
+
     if (auto attr = op->getAttrOfType<spirv::TargetEnvAttr>(
             spirv::getTargetEnvAttrName()))
       return attr;

llvmbot · 2025-07-07T09:33:28Z

@llvm/pr-subscribers-mlir

Author: Jaeho Kim (oojahooo)

Changes

The gpu.module operation can contain spirv.target_env attributes within an array attribute named "targets". So it accounts for that case by iterating over the "targets" attribute, if present, and looking up spirv.target_env.

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

1 Files Affected:

(modified) mlir/lib/Dialect/SPIRV/IR/TargetAndABI.cpp (+9)

diff --git a/mlir/lib/Dialect/SPIRV/IR/TargetAndABI.cpp b/mlir/lib/Dialect/SPIRV/IR/TargetAndABI.cpp
index 5ecbd5d7c59d5..dbaa10e89bd42 100644
--- a/mlir/lib/Dialect/SPIRV/IR/TargetAndABI.cpp
+++ b/mlir/lib/Dialect/SPIRV/IR/TargetAndABI.cpp
@@ -184,6 +184,15 @@ spirv::TargetEnvAttr spirv::lookupTargetEnv(Operation *op) {
     if (!op)
       break;
 
+    if (auto arrAttr = op->getAttrOfType<ArrayAttr>("targets")) {
+      for (auto attr : arrAttr) {
+        if (auto spirvTargetEnvAttr =
+                llvm::dyn_cast<spirv::TargetEnvAttr>(attr)) {
+          return spirvTargetEnvAttr;
+        }
+      }
+    }
+
     if (auto attr = op->getAttrOfType<spirv::TargetEnvAttr>(
             spirv::getTargetEnvAttrName()))
       return attr;

IgWod-IMG

A rather naive question, as I'm not familiar with the GPU dialect. Is it possible for the target env to be present in both "targets" and as an op attribute at the same time? If so, does the look-up order matter? I’d hope the same target env is returned either way, but hypothetically if there is a different env in "targets" and in the op, I wonder which one should take a precedence?

Also, it'd be nice to have a test for the change. Probably in GPU to SPIR-V conversion test directory?

mlir/lib/Dialect/SPIRV/IR/TargetAndABI.cpp

kuhar

Could you add a test that shows where this comes up? I'm not sure if we should look up the target env in"targets" attribute in any of possible parents

It resolves llvm#147262 (comment)

oojahooo · 2025-07-08T09:46:01Z

A rather naive question, as I'm not familiar with the GPU dialect. Is it possible for the target env to be present in both "targets" and as an op attribute at the same time? If so, does the look-up order matter? I’d hope the same target env is returned either way, but hypothetically if there is a different env in "targets" and in the op, I wonder which one should take a precedence?

Also, it'd be nice to have a test for the change. Probably in GPU to SPIR-V conversion test directory?

When the gpu.module op already has a target env as a dict-attr, running the spirv-attach-target pass can reproduce the situation you described. In such cases, I believe that the target env inserted by spirv-attach-target can be occurred without external intervention, so it should be given the highest priority during lookup. If that's the case, I think the flow in the committed code would be appropriate.

Also I added a lit test in GPU to SPIR-V conversion test directory. Thank you for your comment.

oojahooo · 2025-07-08T09:53:19Z

Could you add a test that shows where this comes up? I'm not sure if we should look up the target env in"targets" attribute in any of possible parents

Thank you for your comment. I added a test that reproduces the situation I had in mind.
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.

kuhar · 2025-07-08T14:32:33Z

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?

oojahooo · 2025-07-08T23:49:31Z

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:

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.
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 let me know which 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.

kuhar · 2025-07-09T17:52:19Z

Approach 1. sounds to me like the way to go

…ule` Add lookup target env in "targets" attr logic to GPUToSPIRV pass

oojahooo · 2025-07-10T02:17:04Z

Approach 1. sounds to me like the way to go

Thank you for replying.
I reverted logic of lookupTargetEnv and added new helper function named lookupTargetEnvInTargets, which finds spirv target env in "targets", into GPUToSPIRV code.
Also I added same lookup logic in GPUModuleConversion for remaining target env during conversion.

Happy to make any further changes if needed. Otherwise, would it be okay to merge? Thank you!

mlir/lib/Conversion/GPUToSPIRV/GPUToSPIRVPass.cpp

mlir/lib/Conversion/GPUToSPIRV/GPUToSPIRV.cpp

mlir/lib/Conversion/GPUToSPIRV/GPUToSPIRVPass.cpp

remove unnecessary namespace qualifiers

mlir/lib/Conversion/GPUToSPIRV/GPUToSPIRVPass.cpp

mlir/lib/Conversion/GPUToSPIRV/GPUToSPIRV.cpp

mlir/test/Conversion/GPUToSPIRV/lookup-target-env.mlir

Co-authored-by: Jakub Kuderski <[email protected]>

It had to be checked for a lack of `"targets"` attributes in a GPU module

mlir/lib/Conversion/GPUToSPIRV/GPUToSPIRV.cpp

mlir/lib/Conversion/GPUToSPIRV/GPUToSPIRVPass.cpp

oojahooo · 2025-07-23T23:34:37Z

Hi @kuhar,
I think I've addressed all your previous feedback.
Let me know if there’s anything more you’d like to see changed. Otherwise, I’d appreciate your approval when you get a chance. Thanks!

mlir/lib/Conversion/GPUToSPIRV/GPUToSPIRV.cpp

mlir/test/Conversion/GPUToSPIRV/lookup-target-env.mlir

kuhar

LGTM % one minor request

mlir/test/Conversion/GPUToSPIRV/lookup-target-env.mlir

oojahooo · 2025-07-31T05:50:34Z

Hi @kuhar, thank you again for the approval.
If you have a moment, could you also approve the pending workflows when convenient?
I understand it's quite late in your timezone, so no rush at all, just wanted to leave a gentle note here.

IgWod-IMG

Just a small question, but LGTM otherwise.

IgWod-IMG · 2025-07-31T12:52:03Z

mlir/lib/Conversion/GPUToSPIRV/GPUToSPIRVPass.cpp

+  if (spirv::TargetEnvAttr targetEnvAttr = lookupTargetEnvInTargets(moduleOp))
+    return targetEnvAttr;
+
+  return spirv::lookupTargetEnvOrDefault(moduleOp);


Looking at the tests, it looks like they both test the if statement above. Is there a test checking this part of the function? If not, do we need a test to check that the function behaves as intended when no attributes are attached?

If I understood correctly, we should test the behavior when the gpu.module does not have a targets attribute with spirv target env so that the line 78 code is executed.

I believe this case might already be covered by existing tests. For instance, gpu-to-spirv.mlir tests the behavior when no target env is present at all, and load-store.mlir covers the case where the target env is not attached to thegpu.module's targets attribute but to the module.

However, if a more specific test is needed, such as one where the gpu.module lacks the targets attribute but has a target env in its attr-dict, or if other additional tests are necessary, I would be happy to add them based on your feedback.

If I understood correctly, we should test the behavior when the gpu.module does not have a targets attribute with spirv target env so that the line 78 code is executed.

Correct.

I believe this case might already be covered by existing tests. For instance, gpu-to-spirv.mlir tests the behavior when no target env is present at all, and load-store.mlir covers the case where the target env is not attached to thegpu.module's targets attribute but to the module.

Sounds good, I think we are good.

IgWod-IMG

Thanks for addressing my question. LGTM.

github-actions · 2025-08-01T10:54:21Z

@oojahooo Congratulations on having your first Pull Request (PR) merged into the LLVM Project!

Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR.

Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues.

How to do this, and the rest of the post-merge process, is covered in detail here.

If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again.

If you don't get any reports, no action is required from you. Your changes are working as expected, well done!

oojahooo requested review from antiagainst and kuhar as code owners July 7, 2025 09:32

llvmbot added mlir:spirv mlir labels Jul 7, 2025

IgWod-IMG reviewed Jul 7, 2025

View reviewed changes

mlir/lib/Dialect/SPIRV/IR/TargetAndABI.cpp Outdated Show resolved Hide resolved

kuhar requested changes Jul 7, 2025

View reviewed changes

oojahooo added 2 commits July 8, 2025 18:24

Add test

71d3d92

Delete braces for complying with conding standard

ae6ba9c

It resolves llvm#147262 (comment)

llvmbot added the mlir:gpu label Jul 8, 2025

oojahooo added 3 commits July 10, 2025 11:03

fixup! [mlir][SPIRV] Fix lookup logic spirv.target_env for `gpu.mod…

fa3b44e

…ule` Add lookup target env in "targets" attr logic to GPUToSPIRV pass

Remove unnecessary check in test

7d43c59

Add same lookup target env logic in GPUModuleConversion

40b6f07

IgWod-IMG reviewed Jul 10, 2025

View reviewed changes

fixup! Add same lookup target env logic in GPUModuleConversion

0b68019

remove unnecessary namespace qualifiers

kuhar reviewed Jul 10, 2025

View reviewed changes

kuhar changed the title ~~[mlir][SPIRV] Fix lookup logic spirv.target_env for gpu.module~~ [mlir][spirv] Fix lookup logic spirv.target_env for gpu.module Jul 10, 2025

oojahooo and others added 4 commits July 11, 2025 09:04

Update mlir/test/Conversion/GPUToSPIRV/lookup-target-env.mlir

0833b2a

Co-authored-by: Jakub Kuderski <[email protected]>

Spell out the not obvious types

a688ec2

Add documentation comments

1f38539

Simplify pass pipeline of test code

9344c20

oojahooo requested a review from kuhar July 14, 2025 23:59

oojahooo force-pushed the spirv_lookup_target branch from 1c22145 to 9344c20 Compare July 16, 2025 04:35

Fix failures in other tests

3f8a379

It had to be checked for a lack of `"targets"` attributes in a GPU module

kuhar reviewed Jul 16, 2025

View reviewed changes

mlir/lib/Conversion/GPUToSPIRV/GPUToSPIRV.cpp Outdated Show resolved Hide resolved

mlir/lib/Conversion/GPUToSPIRV/GPUToSPIRVPass.cpp Outdated Show resolved Hide resolved

Do not use reference, do pass by value when assign attributes

3703a1e

kuhar reviewed Jul 24, 2025

View reviewed changes

mlir/lib/Conversion/GPUToSPIRV/GPUToSPIRV.cpp Outdated Show resolved Hide resolved

mlir/test/Conversion/GPUToSPIRV/lookup-target-env.mlir Show resolved Hide resolved

oojahooo added 2 commits July 31, 2025 09:36

Select first target env when targets array has more than one target env

b8b3c18

Add test case with more than one target env in targets array

1a7d496

kuhar approved these changes Jul 31, 2025

View reviewed changes

mlir/test/Conversion/GPUToSPIRV/lookup-target-env.mlir Show resolved Hide resolved

oojahooo added 2 commits July 31, 2025 12:01

Add comment explaining the purpose of new test case

b1e810b

Merge branch 'main' into spirv_lookup_target

6a56521

kuhar requested a review from IgWod-IMG July 31, 2025 12:02

IgWod-IMG reviewed Jul 31, 2025

View reviewed changes

IgWod-IMG approved these changes Aug 1, 2025

View reviewed changes

kuhar merged commit 103461f into llvm:main Aug 1, 2025
9 checks passed

[mlir][spirv] Fix lookup logic spirv.target_env for gpu.module #147262

[mlir][spirv] Fix lookup logic spirv.target_env for gpu.module #147262

Uh oh!

Conversation

oojahooo commented Jul 7, 2025

Uh oh!

github-actions bot commented Jul 7, 2025

Uh oh!

llvmbot commented Jul 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Jul 7, 2025

Uh oh!

IgWod-IMG left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kuhar left a comment

Choose a reason for hiding this comment

Uh oh!

oojahooo commented Jul 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oojahooo commented Jul 8, 2025

Uh oh!

kuhar commented Jul 8, 2025

Uh oh!

oojahooo commented Jul 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kuhar commented Jul 9, 2025

Uh oh!

oojahooo commented Jul 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

oojahooo commented Jul 23, 2025

Uh oh!

Uh oh!

Uh oh!

kuhar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

oojahooo commented Jul 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

IgWod-IMG left a comment

Choose a reason for hiding this comment

Uh oh!

IgWod-IMG Jul 31, 2025

Choose a reason for hiding this comment

Uh oh!

oojahooo Jul 31, 2025

Choose a reason for hiding this comment

Uh oh!

IgWod-IMG Aug 1, 2025

Choose a reason for hiding this comment

Uh oh!

IgWod-IMG left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot commented Aug 1, 2025

Uh oh!

Uh oh!

[mlir][spirv] Fix lookup logic `spirv.target_env` for `gpu.module` #147262

[mlir][spirv] Fix lookup logic `spirv.target_env` for `gpu.module` #147262

llvmbot commented Jul 7, 2025 •

edited

Loading

IgWod-IMG left a comment •

edited

Loading

oojahooo commented Jul 8, 2025 •

edited

Loading

oojahooo commented Jul 8, 2025 •

edited

Loading

oojahooo commented Jul 10, 2025 •

edited

Loading

oojahooo commented Jul 31, 2025 •

edited

Loading