Fix texture-only arg in combine samplers transform.
When a function has an unused texture-only or sampler-only argument,
and no used texture/sampler pairs, the CombineSamplers transform was
failing to remove it from the argument list. The reason is an
incorrect early-out when no texture/sampler pairs are found.
https://dawn-review.googlesource.com/c/dawn/+/154181 attempted to
fix this by adding the unused arguments back as texture/sampler
solo pairs in CollectTextureSamplerPairs(). However, this is
problematic for a number of reasons.
This CL essentially reverts that change, and removes the incorrect
early-out, causing unused texture and sampler parameters to be
(correctly) removed.
Bug: chromium:356298370
Change-Id: If69e4d69dbf308637689622094193a285f9c3a46
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/202155
Reviewed-by: James Price <jrprice@google.com>
Commit-Queue: Stephen White <senorblanco@chromium.org>
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 0bc3df8..583b4b5 100644
--- a/src/tint/lang/glsl/writer/ast_raise/combine_samplers.cc
+++ b/src/tint/lang/glsl/writer/ast_raise/combine_samplers.cc
@@ -46,7 +46,7 @@
namespace {
bool IsGlobal(const tint::sem::VariablePair& pair) {
- return (!pair.first || tint::Is<tint::sem::GlobalVariable>(pair.first)) &&
+ return tint::Is<tint::sem::GlobalVariable>(pair.first) &&
(!pair.second || tint::Is<tint::sem::GlobalVariable>(pair.second));
}
@@ -104,7 +104,7 @@
/// Creates a combined sampler global variables.
/// (Note this is actually a Texture node at the AST level, but it will be
/// written as the corresponding sampler (eg., sampler2D) on GLSL output.)
- /// @param texture_var the texture (global) variable
+ /// @param texture_var the texture (global) variable (must be non-null)
/// @param sampler_var the sampler (global) variable
/// @param name the default name to use (may be overridden by map lookup)
/// @returns the newly-created global variable
@@ -112,11 +112,9 @@
const sem::Variable* sampler_var,
std::string name) {
binding::CombinedTextureSamplerPair st_pair;
- if (texture_var) {
- st_pair.texture = *texture_var->As<sem::GlobalVariable>()->Attributes().binding_point;
- } else {
- st_pair.texture = bindings->placeholder_sampler_bind_point;
- }
+
+ TINT_ASSERT(texture_var);
+ st_pair.texture = *texture_var->As<sem::GlobalVariable>()->Attributes().binding_point;
if (sampler_var) {
st_pair.sampler = *sampler_var->As<sem::GlobalVariable>()->Attributes().binding_point;
@@ -149,24 +147,17 @@
/// 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) {
- 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());
- }
- return CreateASTTypeFor(ctx, texture_type);
+ 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());
}
- TINT_ASSERT(sampler != nullptr);
- const core::type::Type* sampler_type = sampler->Type()->UnwrapRef();
- return CreateASTTypeFor(ctx, sampler_type);
+ return CreateASTTypeFor(ctx, texture_type);
}
/// Insert a new texture/sampler pair into the combined samplers maps (global or local, as
@@ -179,16 +170,9 @@
tint::Vector<const ast::Parameter*, 8>* params) {
const sem::Variable* texture_var = pair.first;
const sem::Variable* sampler_var = pair.second;
- std::string name = "";
-
- if (texture_var) {
- name = texture_var->Declaration()->name->symbol.Name();
- }
+ std::string name = texture_var->Declaration()->name->symbol.Name();
if (sampler_var) {
- if (!name.empty()) {
- name += "_";
- }
- name += sampler_var->Declaration()->name->symbol.Name();
+ name += "_" + sampler_var->Declaration()->name->symbol.Name();
}
if (IsGlobal(pair)) {
// Both texture and sampler are global; add a new global variable
@@ -247,10 +231,6 @@
// separate textures & samplers. Create new combined globals where found.
ctx.ReplaceAll([&](const ast::Function* ast_fn) -> const ast::Function* {
if (auto* fn = sem.Get(ast_fn)) {
- auto pairs = fn->TextureSamplerPairs();
- if (pairs.IsEmpty()) {
- return nullptr;
- }
Vector<const ast::Parameter*, 8> params;
for (auto pair : fn->TextureSamplerPairs()) {
if (!pair.second) {
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 a8f2cb0..61ff10a 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
@@ -1036,14 +1036,12 @@
}
)";
auto* expect = R"(
-fn f(tex_1 : texture_2d<f32>) -> u32 {
+fn f() -> u32 {
return 1u;
}
-@group(0) @binding(0) @internal(disable_validation__binding_point_collision) var t_1 : texture_2d<f32>;
-
fn main() {
- _ = f(t_1);
+ _ = f();
}
)";
@@ -1068,14 +1066,12 @@
}
)";
auto* expect = R"(
-fn f(sampler1_1 : sampler) -> u32 {
+fn f() -> u32 {
return 1u;
}
-@group(0) @binding(0) @internal(disable_validation__binding_point_collision) var s_1 : sampler;
-
fn main() {
- _ = f(s_1);
+ _ = f();
}
)";
@@ -1102,16 +1098,12 @@
}
)";
auto* expect = R"(
-fn f(sampler1_1 : sampler, tex_1 : texture_2d<f32>) -> u32 {
+fn f() -> u32 {
return 1u;
}
-@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 main() {
- _ = f(s_1, t_1);
+ _ = f();
}
)";
@@ -1140,18 +1132,14 @@
}
)";
auto* expect = R"(
-fn f(sampler1_1 : sampler, tex3_1 : texture_2d_array<f32>, tex1_1 : texture_2d<f32>) -> u32 {
+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 s_1 : sampler;
-
@group(0) @binding(0) @internal(disable_validation__binding_point_collision) var t2_1 : texture_2d_array<f32>;
-@group(0) @binding(0) @internal(disable_validation__binding_point_collision) var t1_1 : texture_2d<f32>;
-
fn main() {
- _ = f(s_1, t2_1, t1_1);
+ _ = f(t2_1);
}
)";
@@ -1180,18 +1168,16 @@
}
)";
auto* expect = R"(
-fn f_nested(tex_1 : texture_2d<f32>) -> u32 {
+fn f_nested() -> u32 {
return 1u;
}
-fn f(tex_2 : texture_2d<f32>) -> u32 {
- return f_nested(tex_2);
+fn f() -> u32 {
+ return f_nested();
}
-@group(0) @binding(0) @internal(disable_validation__binding_point_collision) var t_1 : texture_2d<f32>;
-
fn main() {
- _ = f(t_1);
+ _ = f();
}
)";
@@ -1223,20 +1209,59 @@
)";
auto* expect =
R"(
-fn f_nested(sampler1_1 : sampler, tex_1 : texture_2d<f32>) -> u32 {
+fn f_nested() -> u32 {
return 1u;
}
-fn f(sampler1_2 : sampler, tex_2 : texture_2d<f32>) -> u32 {
- return f_nested(sampler1_2, tex_2);
+fn f() -> u32 {
+ return f_nested();
}
-@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 main() {
- _ = f(s_1, t_1);
+ _ = f();
+}
+)";
+
+ ast::transform::DataMap data;
+ Bindings binding;
+ data.Add<Bindings>(binding);
+ auto got = Run<CombineSamplers>(src, data);
+
+ EXPECT_EQ(expect, str(got));
+}
+
+TEST_F(CombineSamplersTest, UnusedTextureAndSamplerFunctionParameter_Nested2) {
+ auto* src = R"(
+@group(0) @binding(1) var texture1 : texture_2d<f32>;
+@group(0) @binding(2) var texture2 : texture_2d<f32>;
+@group(0) @binding(3) var sampler1 : sampler;
+
+fn sample(t : texture_2d<f32>, s : sampler, coords : vec2<f32>, unused : texture_2d<f32>) -> vec4<f32> {
+ return textureSample(t, s, coords);
+}
+
+@fragment
+fn main(@location(0) coords : vec2<f32>) -> @location(0) vec4<f32> {
+ var a = sample(texture1, sampler1, coords, texture1);
+ var b = sample(texture1, sampler1, coords, texture2);
+ return a + b;
+}
+)";
+ auto* expect =
+ R"(
+@group(0) @binding(0) @internal(disable_validation__binding_point_collision) var placeholder_sampler : sampler;
+
+fn sample(t_s : texture_2d<f32>, coords : vec2<f32>) -> vec4<f32> {
+ return textureSample(t_s, placeholder_sampler, coords);
+}
+
+@group(0) @binding(0) @internal(disable_validation__binding_point_collision) var texture1_sampler1 : texture_2d<f32>;
+
+@fragment
+fn main(@location(0) coords : vec2<f32>) -> @location(0) vec4<f32> {
+ var a = sample(texture1_sampler1, coords);
+ var b = sample(texture1_sampler1, coords);
+ return (a + b);
}
)";
diff --git a/src/tint/lang/wgsl/resolver/resolver.cc b/src/tint/lang/wgsl/resolver/resolver.cc
index 6945b28..00657e9 100644
--- a/src/tint/lang/wgsl/resolver/resolver.cc
+++ b/src/tint/lang/wgsl/resolver/resolver.cc
@@ -3143,48 +3143,19 @@
// 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 = As<sem::Parameter>(texture)) {
+ if (auto* param = texture->As<sem::Parameter>()) {
texture = args[param->Index()]->UnwrapLoad()->As<sem::VariableUser>()->Variable();
- texture_sampler_set.Add(texture);
}
- if (auto* param = As<sem::Parameter>(sampler)) {
- sampler = args[param->Index()]->UnwrapLoad()->As<sem::VariableUser>()->Variable();
- texture_sampler_set.Add(sampler);
+ if (sampler) {
+ if (auto* param = sampler->As<sem::Parameter>()) {
+ sampler = args[param->Index()]->UnwrapLoad()->As<sem::VariableUser>()->Variable();
+ }
}
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(param, 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, param);
- texture_sampler_set.Add(sampler);
- }
- }
- }
}
sem::ValueExpression* Resolver::Literal(const ast::LiteralExpression* literal) {
diff --git a/src/tint/lang/wgsl/resolver/resolver_test.cc b/src/tint/lang/wgsl/resolver/resolver_test.cc
index b052239..1c4b861 100644
--- a/src/tint/lang/wgsl/resolver/resolver_test.cc
+++ b/src/tint/lang/wgsl/resolver/resolver_test.cc
@@ -2339,27 +2339,13 @@
EXPECT_TRUE(r()->Resolve()) << r()->error();
auto inner_pairs = Sem().Get(inner_func)->TextureSamplerPairs();
- ASSERT_EQ(inner_pairs.Length(), 2u);
- auto* inner_pair_texture = As<sem::Parameter>(inner_pairs[0].first);
- ASSERT_NE(inner_pair_texture, nullptr);
- EXPECT_EQ(inner_pair_texture->Index(), 1u);
- auto* inner_pair_sampler = As<sem::Parameter>(inner_pairs[1].second);
- ASSERT_NE(inner_pair_sampler, nullptr);
- EXPECT_EQ(inner_pair_sampler->Index(), 2u);
+ ASSERT_EQ(inner_pairs.Length(), 0u);
auto middle_pairs = Sem().Get(middle_func)->TextureSamplerPairs();
- ASSERT_EQ(middle_pairs.Length(), 2u);
- auto* middle_pair_texture = As<sem::Parameter>(middle_pairs[0].first);
- ASSERT_NE(middle_pair_texture, nullptr);
- EXPECT_EQ(middle_pair_texture->Index(), 2u);
- auto* middle_pair_sampler = As<sem::Parameter>(middle_pairs[1].second);
- ASSERT_NE(middle_pair_sampler, nullptr);
- EXPECT_EQ(middle_pair_sampler->Index(), 1u);
+ ASSERT_EQ(middle_pairs.Length(), 0u);
auto outer_pairs = Sem().Get(outer_func)->TextureSamplerPairs();
- ASSERT_EQ(outer_pairs.Length(), 2u);
- EXPECT_TRUE(outer_pairs[0].first != nullptr);
- EXPECT_TRUE(outer_pairs[1].second != nullptr);
+ ASSERT_EQ(outer_pairs.Length(), 0u);
}
TEST_F(ResolverTest, TextureSampler_TextureDimensions) {
diff --git a/src/tint/lang/wgsl/sem/function.h b/src/tint/lang/wgsl/sem/function.h
index 88b3c3d..56d0769 100644
--- a/src/tint/lang/wgsl/sem/function.h
+++ b/src/tint/lang/wgsl/sem/function.h
@@ -142,10 +142,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 (null indicates a sampler-only reference)
+ /// @param texture the texture (must be non-null)
/// @param sampler the sampler (null indicates a texture-only reference)
void AddTextureSamplerPair(const sem::Variable* texture, const sem::Variable* sampler) {
- TINT_ASSERT(texture || sampler);
+ TINT_ASSERT(texture != nullptr);
texture_sampler_pairs_.Add(VariablePair(texture, sampler));
}
