From 470f06a92c32dd91c089c979cba185213f147832 Mon Sep 17 00:00:00 2001 From: Igor Wodiany Date: Fri, 24 Oct 2025 11:59:05 +0100 Subject: [PATCH 1/2] [mlir][spirv] Enable validation of global vars tests According to the SPIR-V spec (3.3.8 Memory Instructions): "Initializer must be an from a constant instruction or a global (module scope) OpVariable instruction." as such it should not be allowed for one `spirv.globalVariable` to initialize another. --- mlir/lib/Dialect/SPIRV/IR/SPIRVOps.cpp | 6 ++--- mlir/test/Dialect/SPIRV/IR/structure-ops.mlir | 2 +- mlir/test/Target/SPIRV/global-variable.mlir | 22 ++++++++----------- 3 files changed, 13 insertions(+), 17 deletions(-) diff --git a/mlir/lib/Dialect/SPIRV/IR/SPIRVOps.cpp b/mlir/lib/Dialect/SPIRV/IR/SPIRVOps.cpp index fe50865bb7c49..650b0e627ad7b 100644 --- a/mlir/lib/Dialect/SPIRV/IR/SPIRVOps.cpp +++ b/mlir/lib/Dialect/SPIRV/IR/SPIRVOps.cpp @@ -1278,10 +1278,10 @@ LogicalResult spirv::GlobalVariableOp::verify() { // TODO: Currently only variable initialization with specialization // constants and other variables is supported. They could be normal // constants in the module scope as well. - if (!initOp || !isa(initOp)) { + if (!initOp || + !isa(initOp)) { return emitOpError("initializer must be result of a " - "spirv.SpecConstant or spirv.GlobalVariable or " + "spirv.SpecConstant or " "spirv.SpecConstantCompositeOp op"); } } diff --git a/mlir/test/Dialect/SPIRV/IR/structure-ops.mlir b/mlir/test/Dialect/SPIRV/IR/structure-ops.mlir index 99ad2a8a2e64b..20bb4eace370b 100644 --- a/mlir/test/Dialect/SPIRV/IR/structure-ops.mlir +++ b/mlir/test/Dialect/SPIRV/IR/structure-ops.mlir @@ -501,7 +501,7 @@ spirv.module Logical GLSL450 { // ----- spirv.module Logical GLSL450 { - // expected-error @+1 {{op initializer must be result of a spirv.SpecConstant or spirv.GlobalVariable or spirv.SpecConstantCompositeOp op}} + // expected-error @+1 {{op initializer must be result of a spirv.SpecConstant or spirv.SpecConstantCompositeOp op}} spirv.GlobalVariable @var0 initializer(@var1) : !spirv.ptr } diff --git a/mlir/test/Target/SPIRV/global-variable.mlir b/mlir/test/Target/SPIRV/global-variable.mlir index a70ed316c68d3..a425412989a83 100644 --- a/mlir/test/Target/SPIRV/global-variable.mlir +++ b/mlir/test/Target/SPIRV/global-variable.mlir @@ -1,11 +1,16 @@ // RUN: mlir-translate -no-implicit-module -test-spirv-roundtrip -split-input-file %s | FileCheck %s +// RUN: %if spirv-tools %{ rm -rf %t %} +// RUN: %if spirv-tools %{ mkdir %t %} +// RUN: %if spirv-tools %{ mlir-translate --no-implicit-module --serialize-spirv --split-input-file --spirv-save-validation-files-with-prefix=%t/module %s %} +// RUN: %if spirv-tools %{ spirv-val %t %} + // CHECK: spirv.GlobalVariable @var0 bind(1, 0) : !spirv.ptr // CHECK-NEXT: spirv.GlobalVariable @var1 bind(0, 1) : !spirv.ptr // CHECK-NEXT: spirv.GlobalVariable @var2 built_in("GlobalInvocationId") : !spirv.ptr, Input> // CHECK-NEXT: spirv.GlobalVariable @var3 built_in("GlobalInvocationId") : !spirv.ptr, Input> -spirv.module Logical GLSL450 requires #spirv.vce { +spirv.module Logical GLSL450 requires #spirv.vce { spirv.GlobalVariable @var0 bind(1, 0) : !spirv.ptr spirv.GlobalVariable @var1 bind(0, 1) : !spirv.ptr spirv.GlobalVariable @var2 {built_in = "GlobalInvocationId"} : !spirv.ptr, Input> @@ -14,16 +19,7 @@ spirv.module Logical GLSL450 requires #spirv.vce { // ----- -spirv.module Logical GLSL450 requires #spirv.vce { - // CHECK: spirv.GlobalVariable @var1 : !spirv.ptr - // CHECK-NEXT: spirv.GlobalVariable @var2 initializer(@var1) bind(1, 0) : !spirv.ptr - spirv.GlobalVariable @var1 : !spirv.ptr - spirv.GlobalVariable @var2 initializer(@var1) bind(1, 0) : !spirv.ptr -} - -// ----- - -spirv.module Logical GLSL450 requires #spirv.vce { +spirv.module Logical GLSL450 requires #spirv.vce { // CHECK: spirv.SpecConstant @sc = 1 : i8 // CHECK-NEXT: spirv.GlobalVariable @var initializer(@sc) : !spirv.ptr spirv.SpecConstant @sc = 1 : i8 @@ -33,7 +29,7 @@ spirv.module Logical GLSL450 requires #spirv.vce { // ----- -spirv.module Logical GLSL450 requires #spirv.vce { +spirv.module Logical GLSL450 requires #spirv.vce { // CHECK: spirv.SpecConstantComposite @scc (@sc0, @sc1, @sc2) : !spirv.array<3 x i8> // CHECK-NEXT: spirv.GlobalVariable @var initializer(@scc) : !spirv.ptr, Uniform> spirv.SpecConstant @sc0 = 1 : i8 @@ -47,7 +43,7 @@ spirv.module Logical GLSL450 requires #spirv.vce { // ----- -spirv.module Logical GLSL450 requires #spirv.vce { +spirv.module Logical GLSL450 requires #spirv.vce { spirv.GlobalVariable @globalInvocationID built_in("GlobalInvocationId") : !spirv.ptr, Input> spirv.func @foo() "None" { // CHECK: %[[ADDR:.*]] = spirv.mlir.addressof @globalInvocationID : !spirv.ptr, Input> From 7ee439bdebbd90e51aa0080d7e32d2874636bcbc Mon Sep 17 00:00:00 2001 From: Igor Wodiany Date: Fri, 24 Oct 2025 16:26:50 +0100 Subject: [PATCH 2/2] Update comments --- mlir/lib/Dialect/SPIRV/IR/SPIRVOps.cpp | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/mlir/lib/Dialect/SPIRV/IR/SPIRVOps.cpp b/mlir/lib/Dialect/SPIRV/IR/SPIRVOps.cpp index 650b0e627ad7b..0c8114d5e957e 100644 --- a/mlir/lib/Dialect/SPIRV/IR/SPIRVOps.cpp +++ b/mlir/lib/Dialect/SPIRV/IR/SPIRVOps.cpp @@ -1276,8 +1276,15 @@ LogicalResult spirv::GlobalVariableOp::verify() { Operation *initOp = SymbolTable::lookupNearestSymbolFrom( (*this)->getParentOp(), init.getAttr()); // TODO: Currently only variable initialization with specialization - // constants and other variables is supported. They could be normal - // constants in the module scope as well. + // constants is supported. There could be normal constants in the module + // scope as well. + // + // In the current setup we also cannot initialize one global variable with + // another. The problem is that if we try to initialize pointer of type X + // with another pointer type, the validator fails because it expects the + // variable to be initialized to be type X, not pointer to X. Now + // `spirv.GlobalVariable` only allows pointer type, so in the current design + // we cannot initialize one `spirv.GlobalVariable` with another. if (!initOp || !isa(initOp)) { return emitOpError("initializer must be result of a "