Skip to content

[mlir][spirv] Fix spirv.Select min version requirement #72173

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Nov 13, 2023

Conversation

antiagainst
Copy link
Member

Per the spec, "Before version 1.4, results are only computed per component." So using scalar condition to select composite needs SPIR-V v1.4 at least.

Per the spec, "Before version 1.4, results are only computed per
component." So using scalar condition to select composite needs
SPIR-V v1.4 at least.
@llvmbot
Copy link
Member

llvmbot commented Nov 13, 2023

@llvm/pr-subscribers-mlir-spirv

@llvm/pr-subscribers-mlir

Author: Lei Zhang (antiagainst)

Changes

Per the spec, "Before version 1.4, results are only computed per component." So using scalar condition to select composite needs SPIR-V v1.4 at least.


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

3 Files Affected:

  • (modified) mlir/include/mlir/Dialect/SPIRV/IR/SPIRVLogicalOps.td (+4)
  • (modified) mlir/lib/Dialect/SPIRV/IR/ControlFlowOps.cpp (+23)
  • (modified) mlir/test/Dialect/SPIRV/Transforms/vce-deduction.mlir (+10)
diff --git a/mlir/include/mlir/Dialect/SPIRV/IR/SPIRVLogicalOps.td b/mlir/include/mlir/Dialect/SPIRV/IR/SPIRVLogicalOps.td
index d6e90724edc2733..cf38c15d20dc326 100644
--- a/mlir/include/mlir/Dialect/SPIRV/IR/SPIRVLogicalOps.td
+++ b/mlir/include/mlir/Dialect/SPIRV/IR/SPIRVLogicalOps.td
@@ -1002,6 +1002,10 @@ def SPIRV_SelectOp : SPIRV_Op<"Select",
   let assemblyFormat = [{
     operands attr-dict `:` type($condition) `,` type($result)
   }];
+
+  // These ops require dynamic availability specification based on operand and
+  // result types.
+  bit autogenAvailability = 0;
 }
 
 // -----
diff --git a/mlir/lib/Dialect/SPIRV/IR/ControlFlowOps.cpp b/mlir/lib/Dialect/SPIRV/IR/ControlFlowOps.cpp
index 081f8b601f41f07..580782043c81b46 100644
--- a/mlir/lib/Dialect/SPIRV/IR/ControlFlowOps.cpp
+++ b/mlir/lib/Dialect/SPIRV/IR/ControlFlowOps.cpp
@@ -10,7 +10,9 @@
 //
 //===----------------------------------------------------------------------===//
 
+#include "mlir/Dialect/SPIRV/IR/SPIRVEnums.h"
 #include "mlir/Dialect/SPIRV/IR/SPIRVOps.h"
+#include "mlir/Dialect/SPIRV/IR/SPIRVTypes.h"
 #include "mlir/Interfaces/CallInterfaces.h"
 
 #include "SPIRVOpUtils.h"
@@ -429,6 +431,27 @@ LogicalResult SelectOp::verify() {
   return success();
 }
 
+// Custom availability implementation is needed for spirv.Select given the
+// syntax changes starting v1.4.
+SmallVector<ArrayRef<spirv::Extension>, 1> SelectOp::getExtensions() {
+  return {};
+}
+SmallVector<ArrayRef<spirv::Capability>, 1> SelectOp::getCapabilities() {
+  return {};
+}
+std::optional<spirv::Version> SelectOp::getMinVersion() {
+  // Per the spec, "Before version 1.4, results are only computed per
+  // component."
+  if (isa<spirv::ScalarType>(getCondition().getType()) &&
+      isa<spirv::CompositeType>(getType()))
+    return Version::V_1_4;
+
+  return Version::V_1_0;
+}
+std::optional<spirv::Version> SelectOp::getMaxVersion() {
+  return Version::V_1_6;
+}
+
 //===----------------------------------------------------------------------===//
 // spirv.mlir.selection
 //===----------------------------------------------------------------------===//
diff --git a/mlir/test/Dialect/SPIRV/Transforms/vce-deduction.mlir b/mlir/test/Dialect/SPIRV/Transforms/vce-deduction.mlir
index 45e6eaa17b3ad69..4eaa21d2f94ef61 100644
--- a/mlir/test/Dialect/SPIRV/Transforms/vce-deduction.mlir
+++ b/mlir/test/Dialect/SPIRV/Transforms/vce-deduction.mlir
@@ -32,6 +32,16 @@ spirv.module Logical GLSL450 attributes {
   }
 }
 
+// CHECK: requires #spirv.vce<v1.4, [Shader], []>
+spirv.module Logical GLSL450 attributes {
+  spirv.target_env = #spirv.target_env<#spirv.vce<v1.6, [Shader], []>, #spirv.resource_limits<>>
+} {
+  spirv.func @select_with_scalar_condition(%predicate : i1, %a: vector<2xf32>, %b: vector<2xf32>) -> vector<2xf32> "None" {
+    %0 = spirv.Select %predicate, %a, %b : i1, vector<2xf32>
+    spirv.ReturnValue %0: vector<2xf32>
+  }
+}
+
 //===----------------------------------------------------------------------===//
 // Capability
 //===----------------------------------------------------------------------===//

Copy link
Contributor

@qedawkins qedawkins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@antiagainst antiagainst merged commit 6d9eb31 into llvm:main Nov 13, 2023
zahiraam pushed a commit to zahiraam/llvm-project that referenced this pull request Nov 20, 2023
Per the spec, "Before version 1.4, results are only computed per
component." So using scalar condition to select composite needs SPIR-V
v1.4 at least.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants