[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!

IgWod-IMG · 2025-07-10T07:57:30Z

mlir/lib/Conversion/GPUToSPIRV/GPUToSPIRVPass.cpp

  bool mapMemorySpace;
 };

+spirv::TargetEnvAttr


Is there a reason to have those two functions? Why not to call spirv::lookupTargetEnvOrDefault directly in GPUToSPIRVPass::lookupTargetEnvInTargets?

Thank you for your comment.

Since I intentionally did not include OrDefault in the function name GPUToSPIRVPass::lookupTargetEnvInTargets, I wanted the function to only attempt to retrieve the target environment from the "targets" attribute, reflecting that limited scope directly in the name.

Then, I defined a separate GPUToSPIRVPass::lookupTargetEnvOrDefault function that calls it and falls back to spirv::lookupTargetEnvOrDefault if needed, making the fallback behavior more explicit.

However, if adding multiple new functions is considered undesirable, I'm happy to simplify the implementation by having only GPUToSPIRVPass::lookupTargetEnvOrDefault and letting it handle whole condition.

However, if adding multiple new functions is considered undesirable, I'm happy to simplify the implementation by having only GPUToSPIRVPass::lookupTargetEnvOrDefault and letting it handle whole condition.

I don't think it's undesirable. I just felt like removing the extra indirection may offer a nicer solution, but you made some good argument about why it makes sense the way it is. Initially I thought that you could add OrDefault in the name of the first function, but now I think it may be more confusing. Let's keep it the way it is, unless someone else has a strong opinion about it.

Thank you for the thoughtful suggestion and for being open to my perspective!

mlir/lib/Conversion/GPUToSPIRV/GPUToSPIRV.cpp

mlir/lib/Conversion/GPUToSPIRV/GPUToSPIRVPass.cpp

remove unnecessary namespace qualifiers

kuhar · 2025-07-10T14:41:18Z

mlir/lib/Conversion/GPUToSPIRV/GPUToSPIRVPass.cpp

  bool mapMemorySpace;
 };

+spirv::TargetEnvAttr
+GPUToSPIRVPass::lookupTargetEnvInTargets(gpu::GPUModuleOp moduleOp) {
+  for (auto &targetAttr : moduleOp.getTargetsAttr())


Spell out the type here since it's not obvious without an IDE/lsp: https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable

kuhar · 2025-07-10T14:41:32Z

mlir/lib/Conversion/GPUToSPIRV/GPUToSPIRVPass.cpp

+
+spirv::TargetEnvAttr
+GPUToSPIRVPass::lookupTargetEnvOrDefault(gpu::GPUModuleOp moduleOp) {
+  if (auto targetEnvAttr = lookupTargetEnvInTargets(moduleOp))


kuhar · 2025-07-10T14:44:12Z

mlir/lib/Conversion/GPUToSPIRV/GPUToSPIRVPass.cpp

+  spirv::TargetEnvAttr lookupTargetEnvInTargets(gpu::GPUModuleOp moduleOp);
+  spirv::TargetEnvAttr lookupTargetEnvOrDefault(gpu::GPUModuleOp moduleOp);


Can you add documentation comments explaining what these do?

kuhar · 2025-07-10T14:44:37Z

mlir/lib/Conversion/GPUToSPIRV/GPUToSPIRV.cpp

@@ -385,6 +385,9 @@ LogicalResult GPUModuleConversion::matchAndRewrite(
  if (auto attr = moduleOp->getAttrOfType<spirv::TargetEnvAttr>(
          spirv::getTargetEnvAttrName()))
    spvModule->setAttr(spirv::getTargetEnvAttrName(), attr);
+  for (auto targetAttr : moduleOp.getTargetsAttr())


Use the actual type here, it's not obvious outside of an IDE: https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable

kuhar · 2025-07-10T14:45:11Z

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

@@ -0,0 +1,17 @@
+// RUN: mlir-opt --spirv-attach-target='caps=Shader exts=SPV_KHR_storage_buffer_storage_class' --convert-gpu-to-spirv %s -o - | FileCheck %s


Suggested change

// RUN: mlir-opt --spirv-attach-target='caps=Shader exts=SPV_KHR_storage_buffer_storage_class' --convert-gpu-to-spirv %s -o - | FileCheck %s

// RUN: mlir-opt --spirv-attach-target='caps=Shader exts=SPV_KHR_storage_buffer_storage_class' --convert-gpu-to-spirv %s | FileCheck %s

kuhar · 2025-07-10T14:46:11Z

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

@@ -0,0 +1,17 @@
+// RUN: mlir-opt --spirv-attach-target='caps=Shader exts=SPV_KHR_storage_buffer_storage_class' --convert-gpu-to-spirv %s -o - | FileCheck %s


What do you think about running convert-gpu-to-spirv only? In the current form, it's not very clear where the 'targets' attribute is materialized.

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

		spirv::TargetEnvAttr lookupTargetEnvInTargets(gpu::GPUModuleOp moduleOp);
		spirv::TargetEnvAttr lookupTargetEnvOrDefault(gpu::GPUModuleOp moduleOp);

		@@ -0,0 +1,17 @@
		// RUN: mlir-opt --spirv-attach-target='caps=Shader exts=SPV_KHR_storage_buffer_storage_class' --convert-gpu-to-spirv %s -o - \| FileCheck %s

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

Are you sure you want to change the base?

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

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kuhar Jul 10, 2025 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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

kuhar Jul 10, 2025 •

edited

Loading