[mlir][spirv] Fix UpdateVCEPass to deduce the correct set of capabilities #151108
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ties When deducing capabilities implied capabilities are not considered, which causes generation of incorrect SPIR-V modules. This commit fixes that by pulling in the capability set all the implied ones. Signed-off-by: Davide Grohmann <[email protected]> Change-Id: Ia30149fb35bbf0071010cb7bc92b86d2e5b6a6af
|
@llvm/pr-subscribers-mlir-spirv @llvm/pr-subscribers-mlir Author: Davide Grohmann (davidegrohmann) ChangesWhen deducing capabilities implied capabilities are not considered, which causes generation of incorrect SPIR-V modules. This commit fixes that by pulling in the capability set all the implied ones. Full diff: https://github.com/llvm/llvm-project/pull/151108.diff 2 Files Affected:
diff --git a/mlir/lib/Dialect/SPIRV/Transforms/UpdateVCEPass.cpp b/mlir/lib/Dialect/SPIRV/Transforms/UpdateVCEPass.cpp
index 6a9b951ca61d6..da316b98c2b20 100644
--- a/mlir/lib/Dialect/SPIRV/Transforms/UpdateVCEPass.cpp
+++ b/mlir/lib/Dialect/SPIRV/Transforms/UpdateVCEPass.cpp
@@ -95,6 +95,16 @@ static LogicalResult checkAndUpdateCapabilityRequirements(
return success();
}
+static SetVector<spirv::Capability>
+withImpliedCapabilities(SetVector<spirv::Capability> &caps) {
+ SetVector<spirv::Capability> allCaps(caps.begin(), caps.end());
+ for (auto cap : caps) {
+ ArrayRef<spirv::Capability> directCaps = getDirectImpliedCapabilities(cap);
+ allCaps.insert(directCaps.begin(), directCaps.end());
+ }
+ return allCaps;
+}
+
void UpdateVCEPass::runOnOperation() {
spirv::ModuleOp module = getOperation();
@@ -168,6 +178,8 @@ void UpdateVCEPass::runOnOperation() {
return WalkResult::interrupt();
}
+ deducedCapabilities = withImpliedCapabilities(deducedCapabilities);
+
return WalkResult::advance();
});
diff --git a/mlir/test/Dialect/SPIRV/Transforms/vce-deduction.mlir b/mlir/test/Dialect/SPIRV/Transforms/vce-deduction.mlir
index 2b237665ffc4a..b536b8e4003f9 100644
--- a/mlir/test/Dialect/SPIRV/Transforms/vce-deduction.mlir
+++ b/mlir/test/Dialect/SPIRV/Transforms/vce-deduction.mlir
@@ -7,7 +7,7 @@
// Test deducing minimal version.
// spirv.IAdd is available from v1.0.
-// CHECK: requires #spirv.vce<v1.0, [Shader], []>
+// CHECK: requires #spirv.vce<v1.0, [Shader, Matrix], []>
spirv.module Logical GLSL450 attributes {
spirv.target_env = #spirv.target_env<
#spirv.vce<v1.5, [Shader], []>, #spirv.resource_limits<>>
@@ -21,7 +21,7 @@ spirv.module Logical GLSL450 attributes {
// Test deducing minimal version.
// spirv.GroupNonUniformBallot is available since v1.3.
-// CHECK: requires #spirv.vce<v1.3, [GroupNonUniformBallot, Shader], []>
+// CHECK: requires #spirv.vce<v1.3, [GroupNonUniformBallot, GroupNonUniform, Shader, Matrix], []>
spirv.module Logical GLSL450 attributes {
spirv.target_env = #spirv.target_env<
#spirv.vce<v1.5, [Shader, GroupNonUniformBallot], []>, #spirv.resource_limits<>>
@@ -32,7 +32,7 @@ spirv.module Logical GLSL450 attributes {
}
}
-// CHECK: requires #spirv.vce<v1.4, [Shader], []>
+// CHECK: requires #spirv.vce<v1.4, [Shader, Matrix], []>
spirv.module Logical GLSL450 attributes {
spirv.target_env = #spirv.target_env<#spirv.vce<v1.6, [Shader], []>, #spirv.resource_limits<>>
} {
@@ -48,7 +48,7 @@ spirv.module Logical GLSL450 attributes {
// Test minimal capabilities.
-// CHECK: requires #spirv.vce<v1.0, [Shader], []>
+// CHECK: requires #spirv.vce<v1.0, [Shader, Matrix], []>
spirv.module Logical GLSL450 attributes {
spirv.target_env = #spirv.target_env<
#spirv.vce<v1.0, [Shader, Float16, Float64, Int16, Int64, VariablePointers], []>, #spirv.resource_limits<>>
@@ -61,7 +61,7 @@ spirv.module Logical GLSL450 attributes {
// Test Physical Storage Buffers are deduced correctly.
-// CHECK: spirv.module PhysicalStorageBuffer64 GLSL450 requires #spirv.vce<v1.0, [PhysicalStorageBufferAddresses, Shader], [SPV_EXT_physical_storage_buffer]>
+// CHECK: spirv.module PhysicalStorageBuffer64 GLSL450 requires #spirv.vce<v1.0, [PhysicalStorageBufferAddresses, Shader, Matrix], [SPV_EXT_physical_storage_buffer]>
spirv.module PhysicalStorageBuffer64 GLSL450 attributes {
spirv.target_env = #spirv.target_env<
#spirv.vce<v1.0, [Shader, PhysicalStorageBufferAddresses], [SPV_EXT_physical_storage_buffer]>, #spirv.resource_limits<>>
@@ -74,7 +74,7 @@ spirv.module PhysicalStorageBuffer64 GLSL450 attributes {
// Test deducing implied capability.
// AtomicStorage implies Shader.
-// CHECK: requires #spirv.vce<v1.0, [Shader], []>
+// CHECK: requires #spirv.vce<v1.0, [Shader, Matrix], []>
spirv.module Logical GLSL450 attributes {
spirv.target_env = #spirv.target_env<
#spirv.vce<v1.0, [AtomicStorage], []>, #spirv.resource_limits<>>
@@ -95,7 +95,7 @@ spirv.module Logical GLSL450 attributes {
// * GroupNonUniformArithmetic
// * GroupNonUniformBallot
-// CHECK: requires #spirv.vce<v1.3, [GroupNonUniformArithmetic, Shader], []>
+// CHECK: requires #spirv.vce<v1.3, [GroupNonUniformArithmetic, GroupNonUniform, Shader, Matrix], []>
spirv.module Logical GLSL450 attributes {
spirv.target_env = #spirv.target_env<
#spirv.vce<v1.3, [Shader, GroupNonUniformArithmetic], []>, #spirv.resource_limits<>>
@@ -106,7 +106,7 @@ spirv.module Logical GLSL450 attributes {
}
}
-// CHECK: requires #spirv.vce<v1.3, [GroupNonUniformClustered, GroupNonUniformBallot, Shader], []>
+// CHECK: requires #spirv.vce<v1.3, [GroupNonUniformClustered, GroupNonUniformBallot, GroupNonUniform, Shader, Matrix], []>
spirv.module Logical GLSL450 attributes {
spirv.target_env = #spirv.target_env<
#spirv.vce<v1.3, [Shader, GroupNonUniformClustered, GroupNonUniformBallot], []>, #spirv.resource_limits<>>
@@ -120,7 +120,7 @@ spirv.module Logical GLSL450 attributes {
// Test type required capabilities
// Using 8-bit integers in non-interface storage class requires Int8.
-// CHECK: requires #spirv.vce<v1.0, [Int8, Shader], []>
+// CHECK: requires #spirv.vce<v1.0, [Int8, Shader, Matrix], []>
spirv.module Logical GLSL450 attributes {
spirv.target_env = #spirv.target_env<
#spirv.vce<v1.3, [Shader, Int8], []>, #spirv.resource_limits<>>
@@ -132,7 +132,7 @@ spirv.module Logical GLSL450 attributes {
}
// Using 16-bit floats in non-interface storage class requires Float16.
-// CHECK: requires #spirv.vce<v1.0, [Float16, Shader], []>
+// CHECK: requires #spirv.vce<v1.0, [Float16, Shader, Matrix], []>
spirv.module Logical GLSL450 attributes {
spirv.target_env = #spirv.target_env<
#spirv.vce<v1.3, [Shader, Float16], []>, #spirv.resource_limits<>>
@@ -144,7 +144,7 @@ spirv.module Logical GLSL450 attributes {
}
// Using 16-element vectors requires Vector16.
-// CHECK: requires #spirv.vce<v1.0, [Vector16, Shader], []>
+// CHECK: requires #spirv.vce<v1.0, [Vector16, Kernel, Shader, Matrix], []>
spirv.module Logical GLSL450 attributes {
spirv.target_env = #spirv.target_env<
#spirv.vce<v1.3, [Shader, Vector16], []>, #spirv.resource_limits<>>
@@ -162,7 +162,7 @@ spirv.module Logical GLSL450 attributes {
// Test deducing minimal extensions.
// spirv.KHR.SubgroupBallot requires the SPV_KHR_shader_ballot extension.
-// CHECK: requires #spirv.vce<v1.0, [SubgroupBallotKHR, Shader], [SPV_KHR_shader_ballot]>
+// CHECK: requires #spirv.vce<v1.0, [SubgroupBallotKHR, Shader, Matrix], [SPV_KHR_shader_ballot]>
spirv.module Logical GLSL450 attributes {
spirv.target_env = #spirv.target_env<
#spirv.vce<v1.0, [Shader, SubgroupBallotKHR],
@@ -193,7 +193,7 @@ spirv.module Logical Vulkan attributes {
// Using 8-bit integers in interface storage class requires additional
// extensions and capabilities.
-// CHECK: requires #spirv.vce<v1.0, [StorageBuffer16BitAccess, Shader, Int16], [SPV_KHR_16bit_storage, SPV_KHR_storage_buffer_storage_class]>
+// CHECK: requires #spirv.vce<v1.0, [StorageBuffer16BitAccess, Shader, Int16, Matrix], [SPV_KHR_16bit_storage, SPV_KHR_storage_buffer_storage_class]>
spirv.module Logical GLSL450 attributes {
spirv.target_env = #spirv.target_env<
#spirv.vce<v1.3, [Shader, StorageBuffer16BitAccess, Int16], []>, #spirv.resource_limits<>>
@@ -208,7 +208,7 @@ spirv.module Logical GLSL450 attributes {
// Complicated nested types
// * Buffer requires ImageBuffer or SampledBuffer.
// * Rg32f requires StorageImageExtendedFormats.
-// CHECK: requires #spirv.vce<v1.0, [UniformAndStorageBuffer8BitAccess, StorageUniform16, Int64, Shader, ImageBuffer, StorageImageExtendedFormats], [SPV_KHR_8bit_storage, SPV_KHR_16bit_storage]>
+// CHECK: requires #spirv.vce<v1.0, [UniformAndStorageBuffer8BitAccess, StorageUniform16, Int64, Shader, StorageBuffer8BitAccess, StorageBuffer16BitAccess, Matrix, ImageBuffer, StorageImageExtendedFormats, SampledBuffer], [SPV_KHR_8bit_storage, SPV_KHR_16bit_storage]>
spirv.module Logical GLSL450 attributes {
spirv.target_env = #spirv.target_env<
#spirv.vce<v1.5, [Shader, UniformAndStorageBuffer8BitAccess, StorageBuffer16BitAccess, StorageUniform16, Int16, Int64, ImageBuffer, StorageImageExtendedFormats], []>,
@@ -219,7 +219,7 @@ spirv.module Logical GLSL450 attributes {
}
// Using bfloat16 requires BFloat16TypeKHR capability and SPV_KHR_bfloat16 extension.
-// CHECK: requires #spirv.vce<v1.0, [StorageBuffer16BitAccess, Shader, BFloat16TypeKHR], [SPV_KHR_bfloat16, SPV_KHR_16bit_storage, SPV_KHR_storage_buffer_storage_class]>
+// CHECK: requires #spirv.vce<v1.0, [StorageBuffer16BitAccess, Shader, BFloat16TypeKHR, Matrix], [SPV_KHR_bfloat16, SPV_KHR_16bit_storage, SPV_KHR_storage_buffer_storage_class]>
spirv.module Logical GLSL450 attributes {
spirv.target_env = #spirv.target_env<
#spirv.vce<v1.0, [Shader, StorageBuffer16BitAccess, BFloat16TypeKHR], [SPV_KHR_bfloat16, SPV_KHR_16bit_storage, SPV_KHR_storage_buffer_storage_class]>,
|
IgWod-IMG
reviewed
Jul 29, 2025
kuhar
reviewed
Jul 29, 2025
Signed-off-by: Davide Grohmann <[email protected]> Change-Id: Ib58ef4d1d24e395678c9527abdd7e96a9b1df9eb
IgWod-IMG
reviewed
Jul 30, 2025
Signed-off-by: Davide Grohmann <[email protected]> Change-Id: I34150644e4bcf559597b3c3b3dbb668e5c828faf
Signed-off-by: Davide Grohmann <[email protected]> Change-Id: Ic8b4f2035d110a659368726be4e4504934541c5c
There is one now. |
kuhar
reviewed
Jul 30, 2025
It is implied by
|
|
This is a bit surprising to me, can you see if we can map it back to the SPIR-V spec document? I wonder if we got it backwards and Matrix implies Shader instead... |
|
It is correct: https://registry.khronos.org/SPIR-V/specs/unified1/SPIRV.html#_capability. Shader implicitly declares Matrix. |
kuhar
approved these changes
Jul 30, 2025
IgWod-IMG
approved these changes
Jul 30, 2025
|
Thanks for the approvals. I cannot merge the patch myself, can someone do that for me? Thanks |
|
Looks like one of the buildbots has failed: https://lab.llvm.org/buildbot/#/builders/55/builds/14938 because |
|
I cannot quickly run asan on my machine, but as a proxy I ran Valgrind and it highlights some invalid reads in the new function, so this may be causing builbot issues. @kuhar does it need to be reverted? |
Could this be related to the fact that we iterate and modify the set at the same time? |
|
I think this is part of the problem; we probably should explicitly recalculate end iterator at every iteration: I think we should revert this PR and fix it without rushing. Are you able to hit revert or does it need commiter rights? If you can't do it, I can do it for you. |
No I cannot revert patches. |
|
I have created a revert PR: #151358. I'll merge once it passes CI. |
|
I have merged the reverted PR, so buildbots should go back to being happy again. I guess we now can fix the issue and re-land this change? It'd be nice to get it to work with a single loop and container (if possible), but I don't mind having a less clever version that we know works. For what's worth I got this change running locally: - for (spirv::Capability cap : caps) {
+ for (unsigned i = 0; i < caps.size(); ++i) {
+ spirv::Capability cap = caps[i];It seems to pass tests and Valgrind doesn't complain. But I'll let you take it from here. Also, I feel like this bug was sort of my fault so sorry for that :) |
llvm-sync bot
pushed a commit
to arm/arm-toolchain
that referenced
this pull request
Jul 30, 2025
…ect set of capabilities" (#151358) Reverts llvm/llvm-project#151108 as it breaks sanitizer builds.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When deducing capabilities implied capabilities are not considered, which causes generation of incorrect SPIR-V modules. This commit fixes that by pulling in the capability set all the implied ones.