spirv-reader: Restrict use of ConstOffset
Only permitted for image sampling (and later gather).
Only permitted for 2D, 2D Array, and 3D textures
Fixed: tint:408
Change-Id: Ib97bd17e45046ec8a2147b46899bf52ad9a8f883
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/35980
Auto-Submit: David Neto <dneto@google.com>
Commit-Queue: Ryan Harrison <rharrison@chromium.org>
Reviewed-by: Ryan Harrison <rharrison@chromium.org>
diff --git a/src/reader/spirv/function.cc b/src/reader/spirv/function.cc
index 97907ab..92f83e5 100644
--- a/src/reader/spirv/function.cc
+++ b/src/reader/spirv/function.cc
@@ -501,6 +501,22 @@
}
// @param opcode a SPIR-V opcode
+// @returns true if the given instruction is an image sampling operation.
+bool IsImageSampling(SpvOp opcode) {
+ switch (opcode) {
+ case SpvOpImageSampleImplicitLod:
+ case SpvOpImageSampleExplicitLod:
+ case SpvOpImageSampleDrefImplicitLod:
+ case SpvOpImageSampleDrefExplicitLod:
+ return true;
+ default:
+ // WGSL doesn't have *Proj* texturing.
+ break;
+ }
+ return false;
+}
+
+// @param opcode a SPIR-V opcode
// @returns true if the given instruction is an image access instruction
// whose first input operand is an OpImage value.
bool IsRawImageAccess(SpvOp opcode) {
@@ -3989,7 +4005,8 @@
params.push_back(create<ast::IdentifierExpression>(
Source{}, ast_module_.RegisterSymbol(name), name));
- if (IsSampledImageAccess(inst.opcode())) {
+ const auto opcode = inst.opcode();
+ if (IsSampledImageAccess(opcode)) {
// Form the sampler operand.
const auto* sampler = parser_impl_.GetMemoryObjectDeclarationForHandle(
image_or_sampled_image_operand_id, false);
@@ -4029,7 +4046,7 @@
std::string builtin_name;
bool use_load_suffix = true;
- switch (inst.opcode()) {
+ switch (opcode) {
case SpvOpImageSampleImplicitLod:
case SpvOpImageSampleExplicitLod:
builtin_name = "textureSample";
@@ -4121,12 +4138,28 @@
}
if (arg_index < num_args &&
(image_operands_mask & SpvImageOperandsConstOffsetMask)) {
+ if (!IsImageSampling(opcode)) {
+ return Fail() << "ConstOffset is only permitted for sampling operations: "
+ << inst.PrettyPrint();
+ }
+ switch (texture_type->dim()) {
+ case ast::type::TextureDimension::k2d:
+ case ast::type::TextureDimension::k2dArray:
+ case ast::type::TextureDimension::k3d:
+ break;
+ default:
+ return Fail() << "ConstOffset is only permitted for 2D, 2D Arrayed, "
+ "and 3D textures: "
+ << inst.PrettyPrint();
+ }
+
params.push_back(ToSignedIfUnsigned(MakeOperand(inst, arg_index)).expr);
image_operands_mask ^= SpvImageOperandsConstOffsetMask;
arg_index++;
}
if (arg_index < num_args &&
(image_operands_mask & SpvImageOperandsSampleMask)) {
+ // TODO(dneto): only permitted with ImageFetch
params.push_back(ToI32(MakeOperand(inst, arg_index)).expr);
image_operands_mask ^= SpvImageOperandsSampleMask;
arg_index++;
@@ -4184,7 +4217,7 @@
value = create<ast::BitcastExpression>(Source{}, result_type, call_expr);
}
if (!expected_component_type->Is<ast::type::F32>() &&
- IsSampledImageAccess(inst.opcode())) {
+ IsSampledImageAccess(opcode)) {
// WGSL permits sampled image access only on float textures.
// Reject this case in the SPIR-V reader, at least until SPIR-V validation
// catches up with this rule and can reject it earlier in the workflow.
diff --git a/src/reader/spirv/parser_impl_handle_test.cc b/src/reader/spirv/parser_impl_handle_test.cc
index 1cf0f1f..d4ad95a 100644
--- a/src/reader/spirv/parser_impl_handle_test.cc
+++ b/src/reader/spirv/parser_impl_handle_test.cc
@@ -30,6 +30,7 @@
using ::testing::Eq;
using ::testing::HasSubstr;
using ::testing::Not;
+using ::testing::StartsWith;
std::string Preamble() {
return R"(
@@ -2617,28 +2618,6 @@
Identifier[not set]{vi12}
Identifier[not set]{vf1234}
)
- })"},
- // OpImageWrite with ConstOffset
- {"%float 2D 0 0 0 2 Rgba32f",
- "OpImageWrite %im %vi12 %vf1234 ConstOffset %offsets2d",
- R"(
- Variable{
- Decorations{
- SetDecoration{2}
- BindingDecoration{1}
- }
- x_20
- uniform_constant
- __storage_texture_write_only_2d_rgba32float
- })",
- R"(Call[not set]{
- Identifier[not set]{textureStore}
- (
- Identifier[not set]{x_20}
- Identifier[not set]{vi12}
- Identifier[not set]{vf1234}
- Identifier[not set]{offsets2d}
- )
})"}}));
INSTANTIATE_TEST_SUITE_P(
@@ -3085,35 +3064,6 @@
}
}
}
- })"},
- // OpImageRead with ConstOffset
- {"%float 2D 0 0 0 2 Rgba32f",
- "%99 = OpImageRead %v4float %im %vi12 ConstOffset %offsets2d",
- R"(Variable{
- Decorations{
- SetDecoration{2}
- BindingDecoration{1}
- }
- x_20
- uniform_constant
- __storage_texture_read_only_2d_rgba32float
- })",
- R"(VariableDeclStatement{
- VariableConst{
- x_99
- none
- __vec_4__f32
- {
- Call[not set]{
- Identifier[not set]{textureLoad}
- (
- Identifier[not set]{x_20}
- Identifier[not set]{vi12}
- Identifier[not set]{offsets2d}
- )
- }
- }
- }
})"}}));
INSTANTIATE_TEST_SUITE_P(
@@ -3147,36 +3097,6 @@
}
}
})"},
- // OpImageFetch with ConstOffset
- // TODO(dneto): Seems this is not valid in WGSL.
- {"%float 2D 0 0 0 1 Unknown",
- "%99 = OpImageFetch %v4float %im %vi12 ConstOffset %offsets2d",
- R"(Variable{
- Decorations{
- SetDecoration{2}
- BindingDecoration{1}
- }
- x_20
- uniform_constant
- __sampled_texture_2d__f32
- })",
- R"(VariableDeclStatement{
- VariableConst{
- x_99
- none
- __vec_4__f32
- {
- Call[not set]{
- Identifier[not set]{textureLoad}
- (
- Identifier[not set]{x_20}
- Identifier[not set]{vi12}
- Identifier[not set]{offsets2d}
- )
- }
- }
- }
- })"},
// OpImageFetch with explicit level
{"%float 2D 0 0 0 1 Unknown",
"%99 = OpImageFetch %v4float %im %vi12 Lod %int_3",
@@ -3760,7 +3680,7 @@
)";
auto p = parser(test::Assemble(assembly));
if (!p->BuildAndParseInternalModule()) {
- EXPECT_THAT(p->error(), Eq(GetParam().expected_error)) << assembly;
+ EXPECT_THAT(p->error(), StartsWith(GetParam().expected_error)) << assembly;
} else {
EXPECT_TRUE(p->error().empty()) << p->error();
FunctionEmitter fe(p.get(), *spirv_function(p.get(), 100));
@@ -4456,6 +4376,59 @@
"sampled image must have float component type",
{}}}));
+INSTANTIATE_TEST_SUITE_P(
+ ConstOffset_BadInstruction_Errors,
+ SpvParserTest_ImageCoordsTest,
+ ::testing::ValuesIn(std::vector<ImageCoordsCase>{
+ // ImageFetch
+ {"%uint 2D 0 0 0 1 Unknown",
+ "%result = OpImageFetch %v4uint %sampled_image %vf12 ConstOffset "
+ "%the_vu12",
+ "ConstOffset is only permitted for sampling operations: ",
+ {}},
+ // ImageRead
+ {"%uint 2D 0 0 0 2 Rgba32ui",
+ "%result = OpImageRead %v4uint %im %vu12 ConstOffset %the_vu12",
+ "ConstOffset is only permitted for sampling operations: ",
+ {}},
+ // ImageWrite
+ {"%uint 2D 0 0 0 2 Rgba32ui",
+ "OpImageWrite %im %vu12 %vu1234 ConstOffset %the_vu12",
+ "ConstOffset is only permitted for sampling operations: ",
+ {}}
+ // TODO(dneto): Gather
+ // TODO(dneto): DrefGather
+ }));
+
+INSTANTIATE_TEST_SUITE_P(
+ ConstOffset_BadDim_Errors,
+ SpvParserTest_ImageCoordsTest,
+ ::testing::ValuesIn(std::vector<ImageCoordsCase>{
+ // 1D
+ {"%uint 1D 0 0 0 1 Unknown",
+ "%result = OpImageSampleImplicitLod %v4float %sampled_image %vf1234 "
+ "ConstOffset %the_vu12",
+ "ConstOffset is only permitted for 2D, 2D Arrayed, and 3D textures: ",
+ {}},
+ // 1D Array
+ {"%uint 1D 0 1 0 1 Unknown",
+ "%result = OpImageSampleImplicitLod %v4float %sampled_image %vf1234 "
+ "ConstOffset %the_vu12",
+ "ConstOffset is only permitted for 2D, 2D Arrayed, and 3D textures: ",
+ {}},
+ // Cube
+ {"%uint Cube 0 0 0 1 Unknown",
+ "%result = OpImageSampleImplicitLod %v4float %sampled_image %vf1234 "
+ "ConstOffset %the_vu12",
+ "ConstOffset is only permitted for 2D, 2D Arrayed, and 3D textures: ",
+ {}},
+ // Cube Array
+ {"%uint Cube 0 1 0 1 Unknown",
+ "%result = OpImageSampleImplicitLod %v4float %sampled_image %vf1234 "
+ "ConstOffset %the_vu12",
+ "ConstOffset is only permitted for 2D, 2D Arrayed, and 3D textures: ",
+ {}}}));
+
} // namespace
} // namespace spirv
} // namespace reader
