[spirv] Always use constants for struct member access We recently introduced a bitcast for access operands to avoid a spirv-val check from firing on negative indices. This bitcast turns some constants into non-constants, which is invalid for struct member access in SPIR-V. This would never be an issue when coming from WGSL because struct member indices always become unsigned values, but the IR supports signed indices too and so the fuzzers found this case. Add an extra case for constants to make sure that they stay as constants to fix this. Fixed: 466695222 Change-Id: I160ad2f305e767e49142b218d1cf0a6f051db96c Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/279475 Reviewed-by: Peter McNeeley <petermcneeley@google.com> Commit-Queue: James Price <jrprice@google.com>
diff --git a/src/tint/lang/spirv/writer/access_test.cc b/src/tint/lang/spirv/writer/access_test.cc index ca3b997..fa72fd8 100644 --- a/src/tint/lang/spirv/writer/access_test.cc +++ b/src/tint/lang/spirv/writer/access_test.cc
@@ -295,6 +295,33 @@ EXPECT_INST("%result_b = OpCompositeExtract %int %str 1 2"); } +TEST_F(SpirvWriterTest, Access_Struct_Value_ConstantIndex_Signed) { + auto* str = ty.Struct(mod.symbols.New("MyStruct"), { + {mod.symbols.Register("a"), ty.i32()}, + {mod.symbols.Register("b"), ty.vec4i()}, + }); + auto* str_val = b.FunctionParam("str", str); + auto* func = b.Function("foo", ty.i32()); + func->SetParams({str_val}); + b.Append(func->Block(), [&] { + auto* result_a = b.Access(ty.i32(), str_val, 0_i); + auto* result_b = b.Access(ty.i32(), str_val, 1_i, 2_i); + b.Return(func, b.Add(result_a, result_b)); + mod.SetName(result_a, "result_a"); + mod.SetName(result_b, "result_b"); + }); + + auto* eb = b.ComputeFunction("main"); + b.Append(eb->Block(), [&] { + b.Let("x", b.Call(func, b.Zero(str))); + b.Return(eb); + }); + + ASSERT_TRUE(Generate()) << Error() << output_; + EXPECT_INST("%result_a = OpCompositeExtract %int %str 0"); + EXPECT_INST("%result_b = OpCompositeExtract %int %str 1 2"); +} + TEST_F(SpirvWriterTest, Access_Struct_Pointer_ConstantIndex) { auto* str = ty.Struct(mod.symbols.New("MyStruct"), { {mod.symbols.Register("a"), ty.i32()}, @@ -323,6 +350,34 @@ EXPECT_INST("%result_b = OpAccessChain %_ptr_Function_v4int %str %uint_1"); } +TEST_F(SpirvWriterTest, Access_Struct_Pointer_ConstantIndex_Signed) { + auto* str = ty.Struct(mod.symbols.New("MyStruct"), { + {mod.symbols.Register("a"), ty.i32()}, + {mod.symbols.Register("b"), ty.vec4i()}, + }); + auto* func = b.Function("foo", ty.vec4i()); + b.Append(func->Block(), [&] { + auto* str_var = b.Var("str", ty.ptr(function, str, read_write)); + auto* result_a = b.Access(ty.ptr<function, i32>(), str_var, 0_i); + auto* result_b = b.Access(ty.ptr<function, vec4<i32>>(), str_var, 1_i); + auto* val_a = b.Load(result_a); + auto* val_b = b.Load(result_b); + b.Return(func, b.Add(val_a, val_b)); + mod.SetName(result_a, "result_a"); + mod.SetName(result_b, "result_b"); + }); + + auto* eb = b.ComputeFunction("main"); + b.Append(eb->Block(), [&] { + b.Let("x", b.Call(func)); + b.Return(eb); + }); + + ASSERT_TRUE(Generate()) << Error() << output_; + EXPECT_INST("%result_a = OpAccessChain %_ptr_Function_int %str %uint_0"); + EXPECT_INST("%result_b = OpAccessChain %_ptr_Function_v4int %str %uint_1"); +} + TEST_F(SpirvWriterTest, LoadVectorElement_ConstantIndex) { auto* func = b.ComputeFunction("main"); b.Append(func->Block(), [&] {
diff --git a/src/tint/lang/spirv/writer/printer/printer.cc b/src/tint/lang/spirv/writer/printer/printer.cc index fbca814..e393e12 100644 --- a/src/tint/lang/spirv/writer/printer/printer.cc +++ b/src/tint/lang/spirv/writer/printer/printer.cc
@@ -1131,9 +1131,16 @@ return Value(idx); } - // If the index isn't a unsigned value, then bitcast it to unsigned. A negative - // value is never allowed as a constant access index in SPIR-V. This cast fixes that + // If the index isn't a unsigned value, then convert it to unsigned. A negative + // value is never allowed as a constant access index in SPIR-V. This conversion fixes that // potential issue. + + // If the index was a constant, keep it as a constant as is required for struct members. + if (auto* c = idx->As<core::ir::Constant>()) { + return Constant(ir_.constant_values.Get(c->Value()->ValueAs<u32>())); + } + + // Use a bitcast for runtime values. uint32_t spv_id = module_.NextId(); current_function_.PushInst(spv::Op::OpBitcast, {Type(ir_.Types().u32()), spv_id, Value(idx)});
diff --git a/test/tint/binding_array/access_as_function_argument.wgsl.expected.spvasm b/test/tint/binding_array/access_as_function_argument.wgsl.expected.spvasm index fe26cae..232e15f 100644 --- a/test/tint/binding_array/access_as_function_argument.wgsl.expected.spvasm +++ b/test/tint/binding_array/access_as_function_argument.wgsl.expected.spvasm
@@ -35,6 +35,7 @@ %v4float = OpTypeVector %float 4 %32 = OpTypeFunction %void %_ptr_UniformConstant_4 = OpTypePointer UniformConstant %4 + %uint_0 = OpConstant %uint 0 %do_texture_load = OpFunction %void None %11 %t = OpFunctionParameter %4 %12 = OpLabel @@ -51,8 +52,7 @@ OpFunctionEnd %fs = OpFunction %void None %32 %33 = OpLabel - %36 = OpBitcast %uint %int_0 - %34 = OpAccessChain %_ptr_UniformConstant_4 %sampled_textures %36 + %34 = OpAccessChain %_ptr_UniformConstant_4 %sampled_textures %uint_0 %37 = OpLoad %4 %34 None %38 = OpFunctionCall %void %do_texture_load %37 OpReturn
diff --git a/test/tint/binding_array/access_constant.wgsl.expected.spvasm b/test/tint/binding_array/access_constant.wgsl.expected.spvasm index e2c4aeb..98f9d08 100644 --- a/test/tint/binding_array/access_constant.wgsl.expected.spvasm +++ b/test/tint/binding_array/access_constant.wgsl.expected.spvasm
@@ -24,9 +24,10 @@ %void = OpTypeVoid %10 = OpTypeFunction %void %_ptr_UniformConstant_4 = OpTypePointer UniformConstant %4 + %uint_0 = OpConstant %uint 0 + %uint_1 = OpConstant %uint 1 %int = OpTypeInt 32 1 %int_0 = OpConstant %int 0 - %uint_1 = OpConstant %uint 1 %v2uint = OpTypeVector %uint 2 %27 = OpConstantComposite %v2uint %uint_1 %uint_1 %v2int = OpTypeVector %int 2 @@ -34,17 +35,16 @@ %v4float = OpTypeVector %float 4 %fs = OpFunction %void None %10 %11 = OpLabel - %14 = OpBitcast %uint %int_0 - %12 = OpAccessChain %_ptr_UniformConstant_4 %sampled_textures %14 - %17 = OpLoad %4 %12 None - %18 = OpImageQueryLevels %uint %17 - %19 = OpISub %uint %18 %uint_1 - %21 = OpBitcast %uint %int_0 - %22 = OpExtInst %uint %23 UMin %21 %19 - %24 = OpImageQuerySizeLod %v2uint %17 %22 + %12 = OpAccessChain %_ptr_UniformConstant_4 %sampled_textures %uint_0 + %15 = OpLoad %4 %12 None + %16 = OpImageQueryLevels %uint %15 + %17 = OpISub %uint %16 %uint_1 + %19 = OpBitcast %uint %int_0 + %22 = OpExtInst %uint %23 UMin %19 %17 + %24 = OpImageQuerySizeLod %v2uint %15 %22 %26 = OpISub %v2uint %24 %27 %28 = OpBitcast %v2uint %29 %31 = OpExtInst %v2uint %23 UMin %28 %26 -%texture_load = OpImageFetch %v4float %17 %31 Lod %22 +%texture_load = OpImageFetch %v4float %15 %31 Lod %22 OpReturn OpFunctionEnd