[tint][resolver] Fix dual source blending validation
The check for an entry point using a non-zero @location and non-zero
@index would only fire if the non-zero index came after the @location.
This now only validates on the entry point (as opposed to a standalone
struct), but given that this is non-spec'd this seems fine.
Rework the validation, and change the diagnostic message to be clearer
about what you did wrong.
Change-Id: I3e35b316814cb149c1f8f77f59e236463d87b2aa
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/160082
Commit-Queue: Ben Clayton <bclayton@google.com>
Kokoro: Kokoro <noreply+kokoro@google.com>
Reviewed-by: James Price <jrprice@google.com>
diff --git a/src/tint/lang/wgsl/resolver/dual_source_blending_extension_test.cc b/src/tint/lang/wgsl/resolver/dual_source_blending_extension_test.cc
index 74451cd..9ea0b05 100644
--- a/src/tint/lang/wgsl/resolver/dual_source_blending_extension_test.cc
+++ b/src/tint/lang/wgsl/resolver/dual_source_blending_extension_test.cc
@@ -107,7 +107,7 @@
Structure("Output", Vector{
Member("a", ty.vec4<f32>(), Vector{Location(0_a), Index(0_a)}),
Member(Source{{12, 34}}, "b", ty.vec4<f32>(),
- Vector{Location(0_a), Index(Source{{12, 34}}, 0_a)}),
+ Vector{Location(Source{{12, 34}}, 0_a), Index(0_a)}),
});
EXPECT_FALSE(r()->Resolve());
@@ -198,22 +198,40 @@
};
// Rendering to multiple render targets while using dual source blending should fail.
-TEST_P(DualSourceBlendingExtensionTestWithParams, MultipleRenderTargetsNotAllowed) {
+TEST_P(DualSourceBlendingExtensionTestWithParams,
+ MultipleRenderTargetsNotAllowed_IndexThenNonZeroLocation) {
Structure("S",
Vector{
Member("a", ty.vec4<f32>(), Vector{Location(0_a), Index(0_a)}),
- Member("b", ty.vec4<f32>(), Vector{Location(0_a), Index(1_a)}),
- Member("c", ty.vec4<f32>(), Vector{Location(Source{{12, 34}}, AInt(GetParam()))}),
+ Member("b", ty.vec4<f32>(), Vector{Location(0_a), Index(Source{{1, 2}}, 1_a)}),
+ Member("c", ty.vec4<f32>(), Vector{Location(Source{{3, 4}}, AInt(GetParam()))}),
});
Func("F", Empty, ty("S"), Vector{Return(Call("S"))},
Vector{Stage(ast::PipelineStage::kFragment)});
EXPECT_FALSE(r()->Resolve());
- StringStream err;
- err << "12:34 error: Multiple render targets are not allowed when using dual source blending. "
- "The output @location("
- << GetParam() << ") is not allowed as a render target.";
- EXPECT_EQ(r()->error(), err.str());
+ EXPECT_EQ(r()->error(),
+ R"(1:2 error: pipeline cannot use both non-zero @index and non-zero @location
+3:4 note: non-zero @location declared here
+note: while analyzing entry point 'F')");
+}
+
+TEST_P(DualSourceBlendingExtensionTestWithParams,
+ MultipleRenderTargetsNotAllowed_NonZeroLocationThenIndex) {
+ Structure("S",
+ Vector{
+ Member("a", ty.vec4<f32>(), Vector{Location(Source{{1, 2}}, AInt(GetParam()))}),
+ Member("b", ty.vec4<f32>(), Vector{Location(0_a), Index(0_a)}),
+ Member("c", ty.vec4<f32>(), Vector{Location(0_a), Index(Source{{3, 4}}, 1_a)}),
+ });
+ Func(Source{{5, 6}}, "F", Empty, ty("S"), Vector{Return(Call("S"))},
+ Vector{Stage(ast::PipelineStage::kFragment)});
+
+ EXPECT_FALSE(r()->Resolve());
+ EXPECT_EQ(r()->error(),
+ R"(3:4 error: pipeline cannot use both non-zero @index and non-zero @location
+1:2 note: non-zero @location declared here
+5:6 note: while analyzing entry point 'F')");
}
INSTANTIATE_TEST_SUITE_P(DualSourceBlendingExtensionTests,
diff --git a/src/tint/lang/wgsl/resolver/validator.cc b/src/tint/lang/wgsl/resolver/validator.cc
index 1a2cff8..67ba42d 100644
--- a/src/tint/lang/wgsl/resolver/validator.cc
+++ b/src/tint/lang/wgsl/resolver/validator.cc
@@ -1113,7 +1113,9 @@
// TODO(jrprice): This state could be stored in sem::Function instead, and then passed to
// sem::Function since it would be useful there too.
Hashset<core::BuiltinValue, 4> builtins;
- Hashset<std::pair<uint32_t, uint32_t>, 8> locationsAndIndexes;
+ Hashset<std::pair<uint32_t, uint32_t>, 8> locations_and_indices;
+ const ast::LocationAttribute* first_nonzero_location = nullptr;
+ const ast::IndexAttribute* first_nonzero_index = nullptr;
enum class ParamOrRetType {
kParameter,
kReturnType,
@@ -1181,15 +1183,8 @@
return LocationAttribute(loc_attr, ty, stage, source);
},
[&](const ast::IndexAttribute* index_attr) {
- bool is_input = param_or_ret == ParamOrRetType::kParameter;
index_attribute = index_attr;
-
- if (TINT_UNLIKELY(!index.has_value())) {
- TINT_ICE() << "@index has no value";
- return false;
- }
-
- return IndexAttribute(index_attr, stage, is_input);
+ return IndexAttribute(index_attr, stage);
},
[&](const ast::InterpolateAttribute* interpolate) {
interpolate_attribute = interpolate;
@@ -1254,20 +1249,28 @@
}
if (location_attribute) {
- uint32_t idx = 0xffffffff;
- if (index_attribute) {
- idx = index.value();
+ if (!first_nonzero_location && location > 0u) {
+ first_nonzero_location = location_attribute;
+ }
+ if (!first_nonzero_index && index > 0u) {
+ first_nonzero_index = index_attribute;
+ }
+ if (first_nonzero_location && first_nonzero_index) {
+ AddError("pipeline cannot use both non-zero @index and non-zero @location",
+ first_nonzero_index->source);
+ AddNote("non-zero @location declared here", first_nonzero_location->source);
+ return false;
}
- std::pair<uint32_t, uint32_t> locationAndIndex(location.value(), idx);
- if (!locationsAndIndexes.Add(locationAndIndex)) {
+ std::pair<uint32_t, uint32_t> location_and_index(location.value(),
+ index.value_or(0));
+ if (!locations_and_indices.Add(location_and_index)) {
StringStream err;
- if (!index_attribute) {
- err << "@location(" << location.value() << ") appears multiple times";
- } else {
- err << "@location(" << location.value() << ") @index(" << index.value()
- << ") appears multiple times";
+ err << "@location(" << location.value() << ") ";
+ if (index_attribute) {
+ err << "@index(" << index.value() << ") ";
}
+ err << "appears multiple times";
AddError(err.str(), location_attribute->source);
return false;
}
@@ -1330,10 +1333,10 @@
for (auto* param : func->Parameters()) {
auto* param_decl = param->Declaration();
+ auto& attrs = param->Attributes();
if (!validate_entry_point_attributes(param_decl->attributes, param->Type(),
param_decl->source, ParamOrRetType::kParameter,
- param->Attributes().location,
- param->Attributes().index)) {
+ attrs.location, attrs.index)) {
return false;
}
}
@@ -1341,7 +1344,7 @@
// Clear IO sets after parameter validation. Builtin and location attributes in return types
// should be validated independently from those used in parameters.
builtins.Clear();
- locationsAndIndexes.Clear();
+ locations_and_indices.Clear();
if (!func->ReturnType()->Is<core::type::Void>()) {
if (!validate_entry_point_attributes(decl->return_type_attributes, func->ReturnType(),
@@ -2150,8 +2153,7 @@
return false;
}
- auto has_index = false;
- Hashset<std::pair<uint32_t, uint32_t>, 8> locationsAndIndexes;
+ Hashset<std::pair<uint32_t, uint32_t>, 8> locations_and_indices;
for (auto* member : str->Members()) {
if (auto* r = member->Type()->As<sem::Array>()) {
if (r->Count()->Is<core::type::RuntimeArrayCount>()) {
@@ -2193,7 +2195,6 @@
},
[&](const ast::IndexAttribute* index) {
index_attribute = index;
- has_index = true;
return IndexAttribute(index, stage);
},
[&](const ast::BuiltinAttribute* builtin_attr) {
@@ -2250,32 +2251,18 @@
// Ensure all locations and index pairs are unique
if (location_attribute) {
- uint32_t index = 0xffffffff;
- if (index_attribute) {
- index = member->Attributes().index.value();
- }
uint32_t location = member->Attributes().location.value();
- if (has_index && location != 0) {
- StringStream err;
- err << "Multiple render targets are not allowed when using dual source blending. "
- "The output @location("
- << location << ") is not allowed as a render target.";
- AddError(err.str(), location_attribute->source);
- return false;
- }
+ uint32_t index = member->Attributes().index.value_or(0);
- std::pair<uint32_t, uint32_t> locationAndIndex(location, index);
- if (!locationsAndIndexes.Add(locationAndIndex)) {
+ std::pair<uint32_t, uint32_t> location_and_index(location, index);
+ if (!locations_and_indices.Add(location_and_index)) {
StringStream err;
- if (!index_attribute) {
- err << "@location(" << location << ") appears multiple times";
- AddError(err.str(), location_attribute->source);
- } else {
- err << "@location(" << location << ") @index(" << index
- << ") appears multiple times";
- AddError(err.str(), index_attribute->source);
+ err << "@location(" << location << ") ";
+ if (index_attribute) {
+ err << "@index(" << index << ") ";
}
-
+ err << "appears multiple times";
+ AddError(err.str(), location_attribute->source);
return false;
}
}
diff --git a/src/tint/lang/wgsl/resolver/validator.h b/src/tint/lang/wgsl/resolver/validator.h
index e78512a..03466ee 100644
--- a/src/tint/lang/wgsl/resolver/validator.h
+++ b/src/tint/lang/wgsl/resolver/validator.h
@@ -344,7 +344,7 @@
/// @param attr the attribute to validate
/// @param type the variable type
/// @param stage the current pipeline stage
- /// @param source the source of the attribute
+ /// @param source the source of declaration using the attribute
/// @returns true on success, false otherwise.
bool LocationAttribute(const ast::LocationAttribute* attr,
const core::type::Type* type,
@@ -359,7 +359,7 @@
/// @returns true on success, false otherwise.
bool IndexAttribute(const ast::IndexAttribute* index_attr,
ast::PipelineStage stage,
- const std::optional<bool> is_input = {}) const;
+ const std::optional<bool> is_input = std::nullopt) const;
/// Validates a loop statement
/// @param stmt the loop statement
