transform::Hlsl: Hoist array initializers out of expressions Move these into a separate const variable declaration statement just above the before the use of the array initializer. HLSL does not allow array initializers as part of a sub-expression Fixed: tint:406 Change-Id: I98f93f2eca172865691831011c852de5e57c5ad6 Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/41484 Commit-Queue: Ben Clayton <bclayton@google.com> Reviewed-by: dan sinclair <dsinclair@chromium.org> Reviewed-by: James Price <jrprice@google.com>
diff --git a/BUILD.gn b/BUILD.gn index 0d68e41..80d4a4f 100644 --- a/BUILD.gn +++ b/BUILD.gn
@@ -1213,6 +1213,7 @@ source_set("tint_unittests_hlsl_writer_src") { sources = [ + "src/transform/hlsl_test.cc", "src/writer/hlsl/generator_impl_alias_type_test.cc", "src/writer/hlsl/generator_impl_array_accessor_test.cc", "src/writer/hlsl/generator_impl_assign_test.cc", @@ -1237,6 +1238,7 @@ "src/writer/hlsl/generator_impl_member_accessor_test.cc", "src/writer/hlsl/generator_impl_module_constant_test.cc", "src/writer/hlsl/generator_impl_return_test.cc", + "src/writer/hlsl/generator_impl_sanitizer_tests.cc", "src/writer/hlsl/generator_impl_switch_test.cc", "src/writer/hlsl/generator_impl_test.cc", "src/writer/hlsl/generator_impl_type_test.cc",
diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index d1648b9..008ce14 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt
@@ -732,6 +732,7 @@ if (${TINT_BUILD_HLSL_WRITER}) list(APPEND TINT_TEST_SRCS + transform/hlsl_test.cc writer/hlsl/generator_impl_alias_type_test.cc writer/hlsl/generator_impl_array_accessor_test.cc writer/hlsl/generator_impl_assign_test.cc @@ -756,6 +757,7 @@ writer/hlsl/generator_impl_member_accessor_test.cc writer/hlsl/generator_impl_module_constant_test.cc writer/hlsl/generator_impl_return_test.cc + writer/hlsl/generator_impl_sanitizer_tests.cc writer/hlsl/generator_impl_switch_test.cc writer/hlsl/generator_impl_test.cc writer/hlsl/generator_impl_type_test.cc
diff --git a/src/clone_context.h b/src/clone_context.h index 0442b3e..39edc4e 100644 --- a/src/clone_context.h +++ b/src/clone_context.h
@@ -108,14 +108,14 @@ // version instead of making yet another copy. auto it = cloned_.find(a); if (it != cloned_.end()) { - return CheckedCast<T>(it->second); + return CheckedCast(it->second); } // First time clone and no replacer transforms matched. // Clone with T::Clone(). auto* c = a->Clone(this); cloned_.emplace(a, c); - return CheckedCast<T>(c); + return CheckedCast(c); } /// Clones the Source `s` into `dst`
diff --git a/src/transform/hlsl.cc b/src/transform/hlsl.cc index acaba88..c75eae5 100644 --- a/src/transform/hlsl.cc +++ b/src/transform/hlsl.cc
@@ -1,4 +1,4 @@ -// Copyright 2020 The Tint Authors. +// Copyright 2021 The Tint Authors. // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -16,7 +16,11 @@ #include <utility> +#include "src/ast/variable_decl_statement.h" #include "src/program_builder.h" +#include "src/semantic/expression.h" +#include "src/semantic/statement.h" +#include "src/type/array_type.h" namespace tint { namespace transform { @@ -26,9 +30,76 @@ Transform::Output Hlsl::Run(const Program* in) { ProgramBuilder out; - CloneContext(&out, in).Clone(); + CloneContext ctx(&out, in); + PromoteArrayInitializerToConstVar(ctx); + ctx.Clone(); return Output{Program(std::move(out))}; } +void Hlsl::PromoteArrayInitializerToConstVar(CloneContext& ctx) const { + // Scan the AST nodes for array initializers which need to be promoted to + // their own constant declaration. + + // Note: Correct handling of arrays-of-arrays is guaranteed due to the + // depth-first traversal of the ast::Node::Clone() methods: + // + // The inner-most array initializers are traversed first, and they are hoisted + // to const variables declared just above the statement of use. The outer + // array initializer will then be hoisted, inserting themselves between the + // inner array declaration and the statement of use. This pattern applies + // correctly to any nested depth. + // + // Depth-first traversal of the AST is guaranteed because AST nodes are fully + // immutable and require their children to be constructed first so their + // pointer can be passed to the parent's constructor. + + for (auto* src_node : ctx.src->ASTNodes().Objects()) { + if (auto* src_init = src_node->As<ast::TypeConstructorExpression>()) { + if (auto* src_sem_expr = ctx.src->Sem().Get(src_init)) { + auto* src_sem_stmt = src_sem_expr->Stmt(); + if (!src_sem_stmt) { + // Expression is outside of a statement. This usually means the + // expression is part of a global (module-scope) constant declaration. + // These must be constexpr, and so cannot contain the type of + // expressions that must be sanitized. + continue; + } + auto* src_stmt = src_sem_stmt->Declaration(); + + if (auto* src_var_decl = src_stmt->As<ast::VariableDeclStatement>()) { + if (src_var_decl->variable()->constructor() == src_init) { + // This statement is just a variable declaration with the array + // initializer as the constructor value. This is what we're + // attempting to transform to, and so ignore. + continue; + } + } + + if (auto* src_array_ty = src_sem_expr->Type()->As<type::Array>()) { + // Create a new symbol for the constant + auto dst_symbol = ctx.dst->Symbols().New(); + // Clone the array type + auto* dst_array_ty = ctx.Clone(src_array_ty); + // Clone the array initializer + auto* dst_init = ctx.Clone(src_init); + // Construct the constant that holds the array + auto* dst_var = ctx.dst->Const( + dst_symbol, ast::StorageClass::kFunction, dst_array_ty, dst_init); + // Construct the variable declaration statement + auto* dst_var_decl = + ctx.dst->create<ast::VariableDeclStatement>(dst_var); + // Construct the identifier for referencing the constant + auto* dst_ident = ctx.dst->Expr(dst_symbol); + + // Insert the constant before the usage + ctx.InsertBefore(src_stmt, dst_var_decl); + // Replace the inlined array with a reference to the constant + ctx.Replace(src_init, dst_ident); + } + } + } + } +} + } // namespace transform } // namespace tint
diff --git a/src/transform/hlsl.h b/src/transform/hlsl.h index c8a04cc..193e54b 100644 --- a/src/transform/hlsl.h +++ b/src/transform/hlsl.h
@@ -18,6 +18,10 @@ #include "src/transform/transform.h" namespace tint { + +// Forward declarations +class CloneContext; + namespace transform { /// Hlsl is a transform used to sanitize a Program for use with the Hlsl writer. @@ -36,6 +40,12 @@ /// @param program the source program to transform /// @returns the transformation result Output Run(const Program* program) override; + + private: + /// Hoists the array initializer to a constant variable, declared just before + /// the array usage statement. + /// See crbug.com/tint/406 for more details + void PromoteArrayInitializerToConstVar(CloneContext& ctx) const; }; } // namespace transform
diff --git a/src/transform/hlsl_test.cc b/src/transform/hlsl_test.cc new file mode 100644 index 0000000..38a4e69 --- /dev/null +++ b/src/transform/hlsl_test.cc
@@ -0,0 +1,154 @@ +// Copyright 2021 The Tint Authors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include "src/transform/hlsl.h" + +#include <memory> +#include <utility> +#include <vector> + +#include "src/transform/test_helper.h" + +namespace tint { +namespace transform { +namespace { + +using HlslTest = TransformTest; + +TEST_F(HlslTest, PromoteArrayInitializerToConstVar_Basic) { + auto* src = R"( +[[stage(vertex)]] +fn main() -> void { + var f0 : f32 = 1.0; + var f1 : f32 = 2.0; + var f2 : f32 = 3.0; + var f3 : f32 = 4.0; + var i : i32 = array<f32, 4>(f0, f1, f2, f3)[2]; +} +)"; + + auto* expect = R"( +[[stage(vertex)]] +fn main() -> void { + var f0 : f32 = 1.0; + var f1 : f32 = 2.0; + var f2 : f32 = 3.0; + var f3 : f32 = 4.0; + const tint_symbol_1 : array<f32, 4> = array<f32, 4>(f0, f1, f2, f3); + var i : i32 = tint_symbol_1[2]; +} +)"; + + auto got = Transform<Hlsl>(src); + + EXPECT_EQ(expect, got); +} + +TEST_F(HlslTest, PromoteArrayInitializerToConstVar_ArrayInArray) { + auto* src = R"( +[[stage(vertex)]] +fn main() -> void { + var i : i32 = array<array<f32, 2>, 2>(array<f32, 2>(1.0, 2.0), array<f32, 2>(3.0, 4.0))[0][1]; +} +)"; + + auto* expect = R"( +[[stage(vertex)]] +fn main() -> void { + const tint_symbol_1 : array<f32, 2> = array<f32, 2>(1.0, 2.0); + const tint_symbol_2 : array<f32, 2> = array<f32, 2>(3.0, 4.0); + const tint_symbol_3 : array<array<f32, 2>, 2> = array<array<f32, 2>, 2>(tint_symbol_1, tint_symbol_2); + var i : i32 = tint_symbol_3[0][1]; +} +)"; + + auto got = Transform<Hlsl>(src); + + EXPECT_EQ(expect, got); +} + +TEST_F(HlslTest, PromoteArrayInitializerToConstVar_NoChangeOnArrayVarDecl) { + auto* src = R"( +[[stage(vertex)]] +fn main() -> void { + var local_arr : array<f32, 4> = array<f32, 4>(0.0, 1.0, 2.0, 3.0); +} + +const module_arr : array<f32, 4> = array<f32, 4>(0.0, 1.0, 2.0, 3.0); +)"; + + auto* expect = src; + + auto got = Transform<Hlsl>(src); + + EXPECT_EQ(expect, got); +} + +TEST_F(HlslTest, PromoteArrayInitializerToConstVar_Bug406) { + // See crbug.com/tint/406 + auto* src = R"( +[[block]] +struct Uniforms { + [[offset(0)]] + transform : mat2x2<f32>; +}; + +[[group(0), binding(0)]] var<uniform> ubo : Uniforms; + +[[builtin(vertex_index)]] var<in> vertex_index : u32; + +[[builtin(position)]] var<out> position : vec4<f32>; + +[[stage(vertex)]] +fn main() -> void { + const transform : mat2x2<f32> = ubo.transform; + var coord : array<vec2<f32>, 3> = array<vec2<f32>, 3>( + vec2<f32>(-1.0, 1.0), + vec2<f32>( 1.0, 1.0), + vec2<f32>(-1.0, -1.0) + )[vertex_index]; + position = vec4<f32>(transform * coord, 0.0, 1.0); +} +)"; + + auto* expect = R"( +[[block]] +struct Uniforms { + [[offset(0)]] + transform : mat2x2<f32>; +}; + +[[group(0), binding(0)]] var<uniform> ubo : Uniforms; + +[[builtin(vertex_index)]] var<in> vertex_index : u32; + +[[builtin(position)]] var<out> position : vec4<f32>; + +[[stage(vertex)]] +fn main() -> void { + const transform : mat2x2<f32> = ubo.transform; + const tint_symbol_1 : array<vec2<f32>, 3> = array<vec2<f32>, 3>(vec2<f32>(-1.0, 1.0), vec2<f32>(1.0, 1.0), vec2<f32>(-1.0, -1.0)); + var coord : array<vec2<f32>, 3> = tint_symbol_1[vertex_index]; + position = vec4<f32>((transform * coord), 0.0, 1.0); +} +)"; + + auto got = Transform<Hlsl>(src); + + EXPECT_EQ(expect, got); +} + +} // namespace +} // namespace transform +} // namespace tint
diff --git a/src/writer/hlsl/generator_impl_sanitizer_tests.cc b/src/writer/hlsl/generator_impl_sanitizer_tests.cc new file mode 100644 index 0000000..b0952ef --- /dev/null +++ b/src/writer/hlsl/generator_impl_sanitizer_tests.cc
@@ -0,0 +1,63 @@ +// Copyright 2021 The Tint Authors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include <memory> +#include <vector> + +#include "src/ast/identifier_expression.h" +#include "src/ast/stage_decoration.h" +#include "src/ast/unary_op_expression.h" +#include "src/ast/variable_decl_statement.h" +#include "src/program.h" +#include "src/writer/hlsl/test_helper.h" + +namespace tint { +namespace writer { +namespace hlsl { +namespace { + +using HlslSanitizerTest = TestHelper; + +TEST_F(HlslSanitizerTest, PromoteArrayInitializerToConstVar) { + auto* array_init = array<i32, 4>(1, 2, 3, 4); + auto* array_index = IndexAccessor(array_init, 3); + auto* pos = Var("pos", ast::StorageClass::kFunction, ty.i32(), array_index); + + Func("main", ast::VariableList{}, ty.void_(), + ast::StatementList{ + create<ast::VariableDeclStatement>(pos), + }, + ast::FunctionDecorationList{ + create<ast::StageDecoration>(ast::PipelineStage::kVertex), + }); + + GeneratorImpl& gen = SanitizeAndBuild(); + + ASSERT_TRUE(gen.Generate(out)) << gen.error(); + + auto got = result(); + auto* expect = R"(void main() { + const int tint_symbol_1[4] = {1, 2, 3, 4}; + int pos = tint_symbol_1[3]; + return; +} + +)"; + EXPECT_EQ(expect, got); +} + +} // namespace +} // namespace hlsl +} // namespace writer +} // namespace tint
diff --git a/src/writer/hlsl/test_helper.h b/src/writer/hlsl/test_helper.h index 01a3905..b49b8e4 100644 --- a/src/writer/hlsl/test_helper.h +++ b/src/writer/hlsl/test_helper.h
@@ -23,6 +23,7 @@ #include "gtest/gtest.h" #include "src/diagnostic/formatter.h" #include "src/program_builder.h" +#include "src/transform/hlsl.h" #include "src/type_determiner.h" #include "src/writer/hlsl/generator_impl.h" @@ -37,7 +38,7 @@ TestHelperBase() = default; ~TestHelperBase() override = default; - /// Builds and returns a GeneratorImpl from the program. + /// Builds the program and returns a GeneratorImpl from the program. /// @note The generator is only built once. Multiple calls to Build() will /// return the same GeneratorImpl without rebuilding. /// @return the built generator @@ -45,19 +46,49 @@ if (gen_) { return *gen_; } + diag::Formatter formatter; [&]() { ASSERT_TRUE(IsValid()) << "Builder program is not valid\n" - << diag::Formatter().format(Diagnostics()); + << formatter.format(Diagnostics()); }(); program = std::make_unique<Program>(std::move(*this)); [&]() { ASSERT_TRUE(program->IsValid()) - << diag::Formatter().format(program->Diagnostics()); + << formatter.format(program->Diagnostics()); }(); gen_ = std::make_unique<GeneratorImpl>(program.get()); return *gen_; } + /// Builds the program, runs the program through the transform::Hlsl sanitizer + /// and returns a GeneratorImpl from the sanitized program. + /// @note The generator is only built once. Multiple calls to Build() will + /// return the same GeneratorImpl without rebuilding. + /// @return the built generator + GeneratorImpl& SanitizeAndBuild() { + if (gen_) { + return *gen_; + } + diag::Formatter formatter; + [&]() { + ASSERT_TRUE(IsValid()) << "Builder program is not valid\n" + << formatter.format(Diagnostics()); + }(); + program = std::make_unique<Program>(std::move(*this)); + [&]() { + ASSERT_TRUE(program->IsValid()) + << formatter.format(program->Diagnostics()); + }(); + auto result = transform::Hlsl().Run(program.get()); + [&]() { + ASSERT_FALSE(result.diagnostics.contains_errors()) + << formatter.format(result.diagnostics); + }(); + *program = std::move(result.program); + gen_ = std::make_unique<GeneratorImpl>(program.get()); + return *gen_; + } + /// @returns the result string std::string result() const { return out.str(); }