Move more validation to functional Pull some of the Function validation out to the functional validator. Change-Id: I5fae66af31349ecad4e8667041030ba78dbe3ccd Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/321555 Reviewed-by: Ryan Harrison <rharrison@chromium.org> Commit-Queue: dan sinclair <dsinclair@chromium.org>
diff --git a/src/tint/lang/core/ir/functional_validator.cc b/src/tint/lang/core/ir/functional_validator.cc index 4279813..978b46a 100644 --- a/src/tint/lang/core/ir/functional_validator.cc +++ b/src/tint/lang/core/ir/functional_validator.cc
@@ -151,10 +151,27 @@ return false; } +/// @returns true if @p ty is a non-struct and decorated with @builtin(position), or if it is a +/// struct and one of its members is decorated, otherwise false. +/// @param attr attributes attached to data +/// @param ty type of the data being tested +bool IsPositionPresent(const IOAttributes& attr, const core::type::Type* ty) { + if (auto* ty_struct = ty->As<core::type::Struct>()) { + for (const auto* mem : ty_struct->Members()) { + if (mem->Attributes().builtin == BuiltinValue::kPosition) { + return true; + } + } + return false; + } + + return attr.builtin == BuiltinValue::kPosition; +} + } // namespace Functional::Functional(const Module& ir, diag::List& diagnostics, ErrorSource error_source) - : ir_(ir), diag_(diagnostics), error_source_(error_source) {} + : ir_(ir), diag_(diagnostics), error_source_(error_source), referenced_module_vars_(ir) {} Functional::~Functional() = default; @@ -281,6 +298,10 @@ return AddNote(src); } +diag::Diagnostic& Functional::AddNote(const Function* func) { + return AddNote(SourceOf(func)); +} + diag::Diagnostic& Functional::AddNote(const Instruction* inst) { return AddNote(SourceOf(inst)); } @@ -302,34 +323,7 @@ } void Functional::CheckFunction(const Function* func) { - if (func->IsEntryPoint()) { - // Check that there is at most one entry point unless we allow multiple entry points. - if (!ir_.properties.Contains(Property::kAllowMultipleEntryPoints)) { - if (!entry_point_names_.IsEmpty()) { - AddError(func) << "a module with multiple entry points requires the " - "AllowMultipleEntryPoints property"; - return; - } - } - - if (DAWN_UNLIKELY(ir_.NameOf(func).Name().empty())) { - AddError(func) << "entry points must have names"; - } else { - // Checking the name early, so its usage can be recorded, even if the function is - // malformed. - const auto name = ir_.NameOf(func).Name(); - if (!entry_point_names_.Add(name)) { - AddError(func) << "entry point name " << style::Function(name) << " is not unique"; - } - } - - if (func->Stage() == Function::PipelineStage::kCompute) { - if (DAWN_UNLIKELY(!func->ReturnType()->Is<core::type::Void>())) { - AddError(func) << "compute entry point must not have a return type, found " - << NameOf(func->ReturnType()); - } - } - } + CheckEntryPoint(func); // void needs to be filtered out, since it isn't constructible, but used in the IR when no // return is specified. @@ -345,6 +339,111 @@ CheckBlock(func->Block()); } +void Functional::CheckEntryPoint(const Function* func) { + if (!func->IsEntryPoint()) { + return; + } + + // Check that there is at most one entry point unless we allow multiple entry points. + if (!ir_.properties.Contains(Property::kAllowMultipleEntryPoints) && + !entry_point_names_.IsEmpty()) { + AddError(func) << "a module with multiple entry points requires the " + "AllowMultipleEntryPoints property"; + return; + } + + if (DAWN_UNLIKELY(ir_.NameOf(func).Name().empty())) { + AddError(func) << "entry points must have names"; + } else { + // Checking the name early, so its usage can be recorded, even if the function is + // malformed. + const auto name = ir_.NameOf(func).Name(); + if (!entry_point_names_.Add(name)) { + AddError(func) << "entry point name " << style::Function(name) << " is not unique"; + } + } + + Hashset<BindingPoint, 4> binding_points{}; + bool seen_immediate = false; + for (auto var : referenced_module_vars_.TransitiveReferences(func)) { + if (!ir_.properties.Contains(Property::kAllowDuplicateBindings) && + var->BindingPoint().has_value()) { + auto bp = var->BindingPoint().value(); + if (!binding_points.Add(bp)) { + AddError(var) << "found non-unique binding point, " << bp + << ", being referenced in entry point, " << NameOf(func); + } + } + + const auto* mv = var->Result()->Type()->As<core::type::MemoryView>(); + if (!mv) { + continue; + } + + auto address_space = mv->AddressSpace(); + switch (address_space) { + case AddressSpace::kImmediate: + if (seen_immediate) { + AddError(var) << "multiple user-declared immediate data variables referenced " + "by entry point " + << NameOf(func); + } + seen_immediate = true; + continue; + case AddressSpace::kWorkgroup: + if (!func->IsCompute()) { + AddError(var) << "workgroup variable cannot be used in a " << func->Stage() + << " shader"; + } + continue; + case AddressSpace::kPixelLocal: + if (!func->IsFragment()) { + AddError(var) << "pixel_local variable cannot be used in a " << func->Stage() + << " shader"; + } + continue; + case AddressSpace::kIn: + case AddressSpace::kOut: + break; + default: + continue; + } + } + + if (func->IsCompute()) { + if (DAWN_UNLIKELY(!func->ReturnType()->Is<core::type::Void>())) { + AddError(func) << "compute entry point must not have a return type, found " + << NameOf(func->ReturnType()); + } + } else if (func->IsVertex()) { + CheckPositionPresentForVertexOutput(func); + } +} + +void Functional::CheckPositionPresentForVertexOutput(const Function* ep) { + if (IsPositionPresent(ep->ReturnAttributes(), ep->ReturnType())) { + return; + } + + for (const auto& var : referenced_module_vars_.TransitiveReferences(ep)) { + const auto* ty = var->Result()->Type()->UnwrapPtrOrRef(); + if (!ty) { + continue; + } + + const auto attr = var->Attributes(); + if (IsPositionPresent(attr, ty)) { + if (!ir_.properties.Contains(Property::kAllowBackendSpecificShaderIO)) { + AddError(var) << "position as part of a `var`, it must be part of the return"; + AddNote(ep) << "used in entry point here"; + return; + } + return; + } + } + AddError(ep) << "position must be declared on the return of a vertex entry point"; +} + void Functional::CheckFunctionParam(const FunctionParam* param) { TINT_ASSERT(param->Function() != nullptr);
diff --git a/src/tint/lang/core/ir/functional_validator.h b/src/tint/lang/core/ir/functional_validator.h index 5363df8..4516748 100644 --- a/src/tint/lang/core/ir/functional_validator.h +++ b/src/tint/lang/core/ir/functional_validator.h
@@ -52,6 +52,7 @@ #include "src/tint/lang/core/ir/member_builtin_call.h" #include "src/tint/lang/core/ir/module.h" #include "src/tint/lang/core/ir/override.h" +#include "src/tint/lang/core/ir/referenced_module_vars.h" #include "src/tint/lang/core/ir/return.h" #include "src/tint/lang/core/ir/store.h" #include "src/tint/lang/core/ir/store_vector_element.h" @@ -115,6 +116,7 @@ diag::Diagnostic& AddNote(Source src); diag::Diagnostic& AddNote(const Block* blk); + diag::Diagnostic& AddNote(const Function* func); diag::Diagnostic& AddNote(const Instruction* inst); diag::Diagnostic& AddNote(const Instruction* inst, size_t idx); @@ -126,6 +128,8 @@ void CheckRootBlock(const Block* blk); void CheckFunction(const Function* func); void CheckFunctionParam(const FunctionParam* param); + void CheckEntryPoint(const Function* func); + void CheckPositionPresentForVertexOutput(const Function* ep); void CheckBlock(const Block* blk); void CheckInstruction(const Instruction* inst); @@ -168,6 +172,7 @@ SymbolTable symbols_ = SymbolTable::Wrap(ir_.symbols); core::type::Manager type_mgr_ = core::type::Manager::Wrap(ir_.Types()); + core::ir::ReferencedModuleVars<const Module> referenced_module_vars_; Vector<const Block*, 8> block_stack_; Hashset<OverrideId, 8> seen_override_ids_;
diff --git a/src/tint/lang/core/ir/structural_validator.cc b/src/tint/lang/core/ir/structural_validator.cc index 8d6569c..2ac1d01 100644 --- a/src/tint/lang/core/ir/structural_validator.cc +++ b/src/tint/lang/core/ir/structural_validator.cc
@@ -113,23 +113,6 @@ return std::string_view(type->TypeInfo().name).starts_with("tint::core"); } -/// @returns true if @p ty is a non-struct and decorated with @builtin(position), or if it is a -/// struct and one of its members is decorated, otherwise false. -/// @param attr attributes attached to data -/// @param ty type of the data being tested -bool IsPositionPresent(const IOAttributes& attr, const core::type::Type* ty) { - if (auto* ty_struct = ty->As<core::type::Struct>()) { - for (const auto* mem : ty_struct->Members()) { - if (mem->Attributes().builtin == BuiltinValue::kPosition) { - return true; - } - } - return false; - } - - return attr.builtin == BuiltinValue::kPosition; -} - template <typename CTX, typename IMPL> void WalkTypeAndMembers(CTX& ctx, const core::type::Type* type, @@ -1352,19 +1335,7 @@ CheckNotBool(f, t, "entry point returns can not be 'bool'"); }); - Hashset<BindingPoint, 4> binding_points{}; - const Var* user_declared_immediate = nullptr; - for (auto var : referenced_module_vars_.TransitiveReferences(func)) { - if (!ir_.properties.Contains(Property::kAllowDuplicateBindings) && - var->BindingPoint().has_value()) { - auto bp = var->BindingPoint().value(); - if (!binding_points.Add(bp)) { - AddError(var) << "found non-unique binding point, " << bp - << ", being referenced in entry point, " << NameOf(func); - } - } - const auto* mv = var->Result()->Type()->As<core::type::MemoryView>(); const auto* ty = var->Result()->Type()->UnwrapPtrOrRef(); const auto attr = var->Attributes(); @@ -1372,28 +1343,7 @@ continue; } - auto address_space = mv->AddressSpace(); - switch (address_space) { - case AddressSpace::kImmediate: - if (user_declared_immediate) { - AddError(var) << "multiple user-declared immediate data variables referenced " - "by entry point " - << NameOf(func); - } - user_declared_immediate = var; - continue; - case AddressSpace::kWorkgroup: - if (!func->IsCompute()) { - AddError(var) << "workgroup variable cannot be used in a " << func->Stage() - << " shader"; - } - continue; - case AddressSpace::kPixelLocal: - if (!func->IsFragment()) { - AddError(var) << "pixel_local variable cannot be used in a " << func->Stage() - << " shader"; - } - continue; + switch (mv->AddressSpace()) { case AddressSpace::kIn: case AddressSpace::kOut: break; @@ -1401,7 +1351,7 @@ continue; } - if (func->IsFragment() && address_space == AddressSpace::kIn) { + if (func->IsFragment() && mv->AddressSpace() == AddressSpace::kIn) { WalkTypeAndMembers(var, ty, attr, [this](const auto* v, const auto* t, const auto& a) { CheckFrontFacingIfBool(v, a, t, "input address space values referenced by fragment shaders " @@ -1417,10 +1367,6 @@ }); } } - - if (func->IsVertex()) { - CheckPositionPresentForVertexOutput(func); - } } bool Structural::CheckFunctionParam(const Function* func, @@ -1880,30 +1826,6 @@ AddError(func) << "@subgroup_size must be an InstructionResult or a Constant"; } -void Structural::CheckPositionPresentForVertexOutput(const Function* ep) { - if (IsPositionPresent(ep->ReturnAttributes(), ep->ReturnType())) { - return; - } - - for (const auto& var : referenced_module_vars_.TransitiveReferences(ep)) { - const auto* ty = var->Result()->Type()->UnwrapPtrOrRef(); - if (!ty) { - continue; - } - - const auto attr = var->Attributes(); - if (IsPositionPresent(attr, ty)) { - if (!ir_.properties.Contains(Property::kAllowBackendSpecificShaderIO)) { - AddError(var) << "position as part of a `var`, it must be part of the return"; - AddNote(ep) << "used in entry point here"; - return; - } - return; - } - } - AddError(ep) << "position must be declared for vertex entry point output"; -} - void Structural::ProcessTasks() { while (!tasks_.IsEmpty()) { tasks_.Pop()();
diff --git a/src/tint/lang/core/ir/validator_function_test.cc b/src/tint/lang/core/ir/validator_function_test.cc index 62209f1..9c1b9e2 100644 --- a/src/tint/lang/core/ir/validator_function_test.cc +++ b/src/tint/lang/core/ir/validator_function_test.cc
@@ -3319,9 +3319,9 @@ auto res = ir::Validate(mod); ASSERT_NE(res, Success); - EXPECT_THAT( - res.Failure().reason, - testing::HasSubstr(R"(:5:1 error: position must be declared for vertex entry point output + EXPECT_THAT(res.Failure().reason, + testing::HasSubstr( + R"(:5:1 error: position must be declared on the return of a vertex entry point %my_func = @vertex func():MyStruct { ^^^^^^^^ )")) << res.Failure(); @@ -3335,9 +3335,9 @@ auto res = ir::Validate(mod); ASSERT_NE(res, Success); - EXPECT_THAT( - res.Failure().reason, - testing::HasSubstr(R"(:1:1 error: position must be declared for vertex entry point output + EXPECT_THAT(res.Failure().reason, + testing::HasSubstr( + R"(:1:1 error: position must be declared on the return of a vertex entry point %my_func = @vertex func():vec4<f32> [@location(0)] { ^^^^^^^^ )")) << res.Failure();