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(); }
