Fix Combine Sampler transform on function with unused tex param Fix by calling AddTextureSamplerPair on all function texture and sampler param to add any potential ones that weren't added by the AddTextureSamplerPair calls for builtins. These parameters could be unused ones, or passed to builtins that are emulated. Bug: tint:2006 Change-Id: I7eff302e7815c92c50f85280d71af8ad80645981 Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/154181 Kokoro: Kokoro <noreply+kokoro@google.com> Reviewed-by: Ben Clayton <bclayton@google.com> Commit-Queue: Shrek Shao <shrekshao@google.com>
diff --git a/src/dawn/tests/end2end/TextureShaderBuiltinTests.cpp b/src/dawn/tests/end2end/TextureShaderBuiltinTests.cpp index 5a2863d..220ef8a 100644 --- a/src/dawn/tests/end2end/TextureShaderBuiltinTests.cpp +++ b/src/dawn/tests/end2end/TextureShaderBuiltinTests.cpp
@@ -179,10 +179,7 @@ @group(0) @binding(2) var tex2 : texture_2d_array<f32>; fn f(tex: texture_2d_array<f32>) -> u32 { - // TODO(tint:2006) Workaround to preserve usage of tex param. Remove when bug is fixed. - var result = textureNumLayers(tex); - result = textureNumLevels(tex); - return result; + return textureNumLevels(tex); } fn f_nested(tex: texture_2d_array<f32>, d: u32) -> u32 {
diff --git a/src/tint/lang/glsl/writer/ast_raise/combine_samplers.cc b/src/tint/lang/glsl/writer/ast_raise/combine_samplers.cc index c00f0fe..3f5ad64 100644 --- a/src/tint/lang/glsl/writer/ast_raise/combine_samplers.cc +++ b/src/tint/lang/glsl/writer/ast_raise/combine_samplers.cc
@@ -33,8 +33,8 @@ namespace { bool IsGlobal(const tint::sem::VariablePair& pair) { - return pair.first->Is<tint::sem::GlobalVariable>() && - (!pair.second || pair.second->Is<tint::sem::GlobalVariable>()); + return (!pair.first || tint::Is<tint::sem::GlobalVariable>(pair.first)) && + (!pair.second || tint::Is<tint::sem::GlobalVariable>(pair.second)); } } // namespace @@ -104,7 +104,9 @@ const sem::Variable* sampler_var, std::string name) { SamplerTexturePair bp_pair; - bp_pair.texture_binding_point = *texture_var->As<sem::GlobalVariable>()->BindingPoint(); + bp_pair.texture_binding_point = + texture_var ? *texture_var->As<sem::GlobalVariable>()->BindingPoint() + : binding_info->placeholder_binding_point; bp_pair.sampler_binding_point = sampler_var ? *sampler_var->As<sem::GlobalVariable>()->BindingPoint() : binding_info->placeholder_binding_point; @@ -132,16 +134,24 @@ /// Creates Identifier for a given texture and sampler variable pair. /// Depth textures with no samplers are turned into the corresponding /// f32 texture (e.g., texture_depth_2d -> texture_2d<f32>). + /// Either texture or sampler could be nullptr, but cannot be nullptr at the same time. + /// The texture can only be nullptr, when the sampler is a dangling function parameter. /// @param texture the texture variable of interest /// @param sampler the texture variable of interest /// @returns the newly-created type ast::Type CreateCombinedASTTypeFor(const sem::Variable* texture, const sem::Variable* sampler) { - const core::type::Type* texture_type = texture->Type()->UnwrapRef(); - const core::type::DepthTexture* depth = texture_type->As<core::type::DepthTexture>(); - if (depth && !sampler) { - return ctx.dst->ty.sampled_texture(depth->dim(), ctx.dst->ty.f32()); + if (texture) { + const core::type::Type* texture_type = texture->Type()->UnwrapRef(); + const core::type::DepthTexture* depth = texture_type->As<core::type::DepthTexture>(); + if (depth && !sampler) { + return ctx.dst->ty.sampled_texture(depth->dim(), ctx.dst->ty.f32()); + } else { + return CreateASTTypeFor(ctx, texture_type); + } } else { - return CreateASTTypeFor(ctx, texture_type); + TINT_ASSERT(sampler != nullptr); + const core::type::Type* sampler_type = sampler->Type()->UnwrapRef(); + return CreateASTTypeFor(ctx, sampler_type); } } @@ -155,9 +165,15 @@ tint::Vector<const ast::Parameter*, 8>* params) { const sem::Variable* texture_var = pair.first; const sem::Variable* sampler_var = pair.second; - std::string name = texture_var->Declaration()->name->symbol.Name(); + std::string name = ""; + if (texture_var) { + name = texture_var->Declaration()->name->symbol.Name(); + } if (sampler_var) { - name += "_" + sampler_var->Declaration()->name->symbol.Name(); + if (!name.empty()) { + name += "_"; + } + name += sampler_var->Declaration()->name->symbol.Name(); } if (IsGlobal(pair)) { // Both texture and sampler are global; add a new global variable
diff --git a/src/tint/lang/glsl/writer/ast_raise/combine_samplers_test.cc b/src/tint/lang/glsl/writer/ast_raise/combine_samplers_test.cc index d408a8b..a324e24 100644 --- a/src/tint/lang/glsl/writer/ast_raise/combine_samplers_test.cc +++ b/src/tint/lang/glsl/writer/ast_raise/combine_samplers_test.cc
@@ -982,5 +982,180 @@ EXPECT_EQ(expect, str(got)); } +TEST_F(CombineSamplersTest, UnusedTextureFunctionParameter) { + auto* src = R"( +@group(0) @binding(0) var t : texture_2d<f32>; + +fn f(tex: texture_2d<f32>) -> u32 { + return 1u; +} + +fn main() { + _ = f(t); +} +)"; + auto* expect = R"( +@group(0) @binding(0) @internal(disable_validation__binding_point_collision) var t_1 : texture_2d<f32>; + +fn f() -> u32 { + return 1u; +} + +fn main() { + _ = f(); +} +)"; + + ast::transform::DataMap data; + data.Add<CombineSamplers::BindingInfo>(CombineSamplers::BindingMap(), BindingPoint()); + auto got = Run<CombineSamplers>(src, data); + + EXPECT_EQ(expect, str(got)); +} + +TEST_F(CombineSamplersTest, UnusedSamplerFunctionParameter) { + auto* src = R"( +@group(0) @binding(0) var s : sampler; + +fn f(sampler1: sampler) -> u32 { + return 1u; +} + +fn main() { + _ = f(s); +} +)"; + auto* expect = R"( +@group(0) @binding(0) @internal(disable_validation__binding_point_collision) var s_1 : sampler; + +fn f() -> u32 { + return 1u; +} + +fn main() { + _ = f(); +} +)"; + + ast::transform::DataMap data; + data.Add<CombineSamplers::BindingInfo>(CombineSamplers::BindingMap(), BindingPoint()); + auto got = Run<CombineSamplers>(src, data); + + EXPECT_EQ(expect, str(got)); +} + +TEST_F(CombineSamplersTest, UnusedTextureAndSamplerFunctionParameter) { + auto* src = R"( +@group(0) @binding(0) var t : texture_2d<f32>; + +@group(0) @binding(1) var s : sampler; + +fn f(tex: texture_2d<f32>, sampler1: sampler) -> u32 { + return 1u; +} + +fn main() { + _ = f(t, s); +} +)"; + auto* expect = R"( +@group(0) @binding(0) @internal(disable_validation__binding_point_collision) var s_1 : sampler; + +@group(0) @binding(0) @internal(disable_validation__binding_point_collision) var t_1 : texture_2d<f32>; + +fn f() -> u32 { + return 1u; +} + +fn main() { + _ = f(); +} +)"; + + ast::transform::DataMap data; + data.Add<CombineSamplers::BindingInfo>(CombineSamplers::BindingMap(), BindingPoint()); + auto got = Run<CombineSamplers>(src, data); + + EXPECT_EQ(expect, str(got)); +} + +TEST_F(CombineSamplersTest, UnusedTextureFunctionParameter_Multiple) { + auto* src = R"( +@group(0) @binding(0) var t1 : texture_2d<f32>; + +@group(0) @binding(1) var t2 : texture_2d_array<f32>; + +@group(0) @binding(2) var s : sampler; + +fn f(tex1: texture_2d<f32>, tex2: texture_2d<f32>, tex3: texture_2d_array<f32>, sampler1: sampler) -> u32 { + return 1u + textureNumLayers(tex3); +} + +fn main() { + _ = f(t1, t1, t2, s); +} +)"; + auto* expect = R"( +@group(0) @binding(0) @internal(disable_validation__binding_point_collision) var s_1 : sampler; + +@group(0) @binding(0) @internal(disable_validation__binding_point_collision) var t1_1 : texture_2d<f32>; + +fn f(tex3_1 : texture_2d_array<f32>) -> u32 { + return (1u + textureNumLayers(tex3_1)); +} + +@group(0) @binding(0) @internal(disable_validation__binding_point_collision) var t2_1 : texture_2d_array<f32>; + +fn main() { + _ = f(t2_1); +} +)"; + + ast::transform::DataMap data; + data.Add<CombineSamplers::BindingInfo>(CombineSamplers::BindingMap(), BindingPoint()); + auto got = Run<CombineSamplers>(src, data); + + EXPECT_EQ(expect, str(got)); +} + +TEST_F(CombineSamplersTest, UnusedTextureFunctionParameter_Nested) { + auto* src = R"( +@group(0) @binding(0) var t : texture_2d<f32>; + +fn f_nested(tex: texture_2d<f32>) -> u32 { + return 1u; +} + +fn f(tex: texture_2d<f32>) -> u32 { + return f_nested(tex); +} + +fn main() { + _ = f(t); +} +)"; + auto* expect = R"( +fn f_nested(tex_1 : texture_2d<f32>) -> u32 { + return 1u; +} + +fn f(tex_2 : texture_2d<f32>) -> u32 { + return f_nested(tex_2); +} + +@group(0) @binding(0) @internal(disable_validation__binding_point_collision) var t_1 : texture_2d<f32>; + +fn main() { + _ = f(t_1); +} +)"; + + ast::transform::DataMap data; + data.Add<CombineSamplers::BindingInfo>(CombineSamplers::BindingMap(), BindingPoint()); + auto got = Run<CombineSamplers>(src, data); + + EXPECT_EQ(expect, str(got)); +} + } // namespace } // namespace tint::glsl::writer
diff --git a/src/tint/lang/wgsl/resolver/resolver.cc b/src/tint/lang/wgsl/resolver/resolver.cc index 4b7c287..9f6843f 100644 --- a/src/tint/lang/wgsl/resolver/resolver.cc +++ b/src/tint/lang/wgsl/resolver/resolver.cc
@@ -3078,19 +3078,48 @@ // argument passed to the current function. Leave global variables // as-is. Then add the mapped pair to the current function's list of // texture/sampler pairs. + + Hashset<const sem::Variable*, 4> texture_sampler_set; + for (sem::VariablePair pair : func->TextureSamplerPairs()) { const sem::Variable* texture = pair.first; const sem::Variable* sampler = pair.second; - if (auto* param = texture->As<sem::Parameter>()) { + if (auto* param = As<sem::Parameter>(texture)) { texture = args[param->Index()]->UnwrapLoad()->As<sem::VariableUser>()->Variable(); + texture_sampler_set.Add(texture); } - if (sampler) { - if (auto* param = sampler->As<sem::Parameter>()) { - sampler = args[param->Index()]->UnwrapLoad()->As<sem::VariableUser>()->Variable(); - } + if (auto* param = As<sem::Parameter>(sampler)) { + sampler = args[param->Index()]->UnwrapLoad()->As<sem::VariableUser>()->Variable(); + texture_sampler_set.Add(sampler); } current_function_->AddTextureSamplerPair(texture, sampler); } + + // Add any possible texture/sampler not essentially passed to builtins from the function param. + // This could be unused texture/sampler or texture/sampler passed to builtins that are emulated. + + const auto& signature = func->Signature(); + + for (size_t i = 0; i < signature.parameters.Length(); i++) { + auto* param = signature.parameters[i]; + if (param->Type()->Is<core::type::Texture>()) { + auto* user = args[i]->UnwrapLoad()->As<sem::VariableUser>(); + auto* texture = user->Variable(); + if (!texture_sampler_set.Contains(texture)) { + current_function_->AddTextureSamplerPair(texture, nullptr); + func->AddTextureSamplerPair(texture, nullptr); + texture_sampler_set.Add(texture); + } + } else if (param->Type()->Is<core::type::Sampler>()) { + auto* user = args[i]->UnwrapLoad()->As<sem::VariableUser>(); + auto* sampler = user->Variable(); + if (!texture_sampler_set.Contains(sampler)) { + current_function_->AddTextureSamplerPair(nullptr, sampler); + func->AddTextureSamplerPair(nullptr, sampler); + texture_sampler_set.Add(sampler); + } + } + } } sem::ValueExpression* Resolver::Literal(const ast::LiteralExpression* literal) {
diff --git a/src/tint/lang/wgsl/sem/function.h b/src/tint/lang/wgsl/sem/function.h index c0da248..8d01a14 100644 --- a/src/tint/lang/wgsl/sem/function.h +++ b/src/tint/lang/wgsl/sem/function.h
@@ -129,9 +129,10 @@ /// that this function uses (directly or indirectly). These can only /// be parameters to this function or global variables. Uniqueness is /// ensured by texture_sampler_pairs_ being a UniqueVector. - /// @param texture the texture (must be non-null) + /// @param texture the texture (null indicates a sampler-only reference) /// @param sampler the sampler (null indicates a texture-only reference) void AddTextureSamplerPair(const sem::Variable* texture, const sem::Variable* sampler) { + TINT_ASSERT(texture || sampler); texture_sampler_pairs_.Add(VariablePair(texture, sampler)); }