Add transform::VarForDynamicIndex

Use this as part of the Spirv sanitizer.
Cleans up buggy dynamic array indexing logic in the SPIR-V writer.

Fixed: tint:824
Change-Id: Ia408e49bd808bc8dbf3a1897eb47f9b33b71fdfb
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/51780
Commit-Queue: Ben Clayton <bclayton@google.com>
Commit-Queue: Antonio Maiorano <amaiorano@google.com>
Auto-Submit: Ben Clayton <bclayton@google.com>
Reviewed-by: Antonio Maiorano <amaiorano@google.com>
This commit is contained in:
Ben Clayton
2021-05-20 11:55:28 +00:00
committed by Tint LUCI CQ
parent c5f5d3641c
commit 9e32b20096
15 changed files with 563 additions and 150 deletions

View File

@@ -523,6 +523,8 @@ libtint_source_set("libtint_core_all_src") {
"transform/single_entry_point.h",
"transform/transform.cc",
"transform/transform.h",
"transform/var_for_dynamic_index.cc",
"transform/var_for_dynamic_index.h",
"transform/vertex_pulling.cc",
"transform/vertex_pulling.h",
"typepair.h",

View File

@@ -287,6 +287,8 @@ set(TINT_LIB_SRCS
transform/single_entry_point.h
transform/transform.cc
transform/transform.h
transform/var_for_dynamic_index.cc
transform/var_for_dynamic_index.h
transform/vertex_pulling.cc
transform/vertex_pulling.h
sem/bool_type.cc
@@ -810,6 +812,7 @@ if(${TINT_BUILD_TESTS})
transform/renamer_test.cc
transform/single_entry_point.cc
transform/test_helper.h
transform/var_for_dynamic_index_test.cc
transform/vertex_pulling_test.cc
)
endif()

View File

@@ -28,6 +28,7 @@
#include "src/sem/variable.h"
#include "src/transform/external_texture_transform.h"
#include "src/transform/manager.h"
#include "src/transform/var_for_dynamic_index.h"
TINT_INSTANTIATE_TYPEINFO(tint::transform::Spirv::Config);
@@ -40,6 +41,7 @@ Spirv::~Spirv() = default;
Output Spirv::Run(const Program* in, const DataMap& data) {
Manager manager;
manager.Add<ExternalTextureTransform>();
manager.Add<VarForDynamicIndex>();
auto transformedInput = manager.Run(in, data);
auto* cfg = data.Get<Config>();

View File

@@ -0,0 +1,80 @@
// 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/var_for_dynamic_index.h"
#include <utility>
#include "src/program_builder.h"
#include "src/sem/array.h"
#include "src/sem/block_statement.h"
#include "src/sem/expression.h"
#include "src/sem/statement.h"
namespace tint {
namespace transform {
VarForDynamicIndex::VarForDynamicIndex() = default;
VarForDynamicIndex::~VarForDynamicIndex() = default;
Output VarForDynamicIndex::Run(const Program* in, const DataMap&) {
ProgramBuilder out;
CloneContext ctx(&out, in);
for (auto* node : in->ASTNodes().Objects()) {
if (auto* access_expr = node->As<ast::ArrayAccessorExpression>()) {
// Found an array accessor expression
auto* index_expr = access_expr->idx_expr();
auto* array_expr = access_expr->array();
if (index_expr->Is<ast::ScalarConstructorExpression>()) {
// Index expression is a literal value. As this isn't a dynamic index,
// we can ignore this.
continue;
}
auto* array = ctx.src->Sem().Get(array_expr);
if (!array->Type()->Is<sem::Array>()) {
// This transform currently only cares about arrays.
continue;
}
auto* stmt = array->Stmt(); // Statement that owns the expression
auto* block = stmt->Block(); // Block that owns the statement
// Construct a `var` declaration to hold the value in memory.
auto* ty = CreateASTTypeFor(&ctx, array->Type());
auto var_name = ctx.dst->Symbols().New("var_for_array");
auto* var_decl = ctx.dst->Decl(ctx.dst->Var(
var_name, ty, ast::StorageClass::kNone, ctx.Clone(array_expr)));
// Insert the `var` declaration before the statement that performs the
// indexing. Note that for indexing chains, AST node ordering guarantees
// that the inner-most index variable will be placed first in the block.
ctx.InsertBefore(block->Declaration()->statements(), stmt->Declaration(),
var_decl);
// Replace the original index expression with the new `var`.
ctx.Replace(array_expr, ctx.dst->Expr(var_name));
}
}
ctx.Clone();
return Output(Program(std::move(out)));
}
} // namespace transform
} // namespace tint

View File

@@ -0,0 +1,48 @@
// 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.
#ifndef SRC_TRANSFORM_VAR_FOR_DYNAMIC_INDEX_H_
#define SRC_TRANSFORM_VAR_FOR_DYNAMIC_INDEX_H_
#include <string>
#include <unordered_map>
#include "src/transform/transform.h"
namespace tint {
namespace transform {
/// A transform that extracts array values that are dynamically indexed to a
/// temporary `var` local before performing the index. This transform is used by
/// the SPIR-V writer for dynamically indexing arrays, as there is no SPIR-V
/// instruction that can dynamically index a non-pointer composite.
class VarForDynamicIndex : public Transform {
public:
/// Constructor
VarForDynamicIndex();
/// Destructor
~VarForDynamicIndex() override;
/// Runs the transform on `program`, returning the transformation result.
/// @param program the source program to transform
/// @param data optional extra transform-specific input data
/// @returns the transformation result
Output Run(const Program* program, const DataMap& data = {}) override;
};
} // namespace transform
} // namespace tint
#endif // SRC_TRANSFORM_VAR_FOR_DYNAMIC_INDEX_H_

View File

@@ -0,0 +1,121 @@
// 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/var_for_dynamic_index.h"
#include "src/transform/test_helper.h"
namespace tint {
namespace transform {
namespace {
using VarForDynamicIndexTest = TransformTest;
TEST_F(VarForDynamicIndexTest, EmptyModule) {
auto* src = "";
auto* expect = "";
auto got = Run<VarForDynamicIndex>(src);
EXPECT_EQ(expect, str(got));
}
TEST_F(VarForDynamicIndexTest, ArrayIndexDynamic) {
auto* src = R"(
fn f() {
var i : i32;
let p : array<i32, 4> = array<i32, 4>(1, 2, 3, 4);
let x : i32 = p[i];
}
)";
auto* expect = R"(
fn f() {
var i : i32;
let p : array<i32, 4> = array<i32, 4>(1, 2, 3, 4);
var var_for_array : array<i32, 4> = p;
let x : i32 = var_for_array[i];
}
)";
auto got = Run<VarForDynamicIndex>(src);
EXPECT_EQ(expect, str(got));
}
TEST_F(VarForDynamicIndexTest, ArrayIndexDynamicChain) {
auto* src = R"(
fn f() {
var i : i32;
var j : i32;
let p : array<array<i32, 2>, 2> = array<array<i32, 2>, 2>(array<i32, 2>(1, 2), array<i32, 2>(3, 4));
let x : i32 = p[i][j];
}
)";
// TODO(bclayton): Optimize this case:
// This output is not as efficient as it could be.
// We only actually need to hoist the inner-most array to a `var`
// (`var_for_array`), as later indexing operations will be working with
// references, not values.
auto* expect = R"(
fn f() {
var i : i32;
var j : i32;
let p : array<array<i32, 2>, 2> = array<array<i32, 2>, 2>(array<i32, 2>(1, 2), array<i32, 2>(3, 4));
var var_for_array : array<array<i32, 2>, 2> = p;
var var_for_array_1 : array<i32, 2> = var_for_array[i];
let x : i32 = var_for_array_1[j];
}
)";
auto got = Run<VarForDynamicIndex>(src);
EXPECT_EQ(expect, str(got));
}
TEST_F(VarForDynamicIndexTest, ArrayIndexLiteral) {
auto* src = R"(
fn f() {
let p : array<i32, 4> = array<i32, 4>(1, 2, 3, 4);
let x : i32 = p[1];
}
)";
auto* expect = src;
auto got = Run<VarForDynamicIndex>(src);
EXPECT_EQ(expect, str(got));
}
TEST_F(VarForDynamicIndexTest, ArrayIndexLiteralChain) {
auto* src = R"(
fn f() {
let p : array<array<i32, 2>, 2> = array<array<i32, 2>, 2>(array<i32, 2>(1, 2), array<i32, 2>(3, 4));
let x : i32 = p[0][1];
}
)";
auto* expect = src;
auto got = Run<VarForDynamicIndex>(src);
EXPECT_EQ(expect, str(got));
}
} // namespace
} // namespace transform
} // namespace tint

View File

@@ -1073,53 +1073,17 @@ uint32_t Builder::GenerateAccessorExpression(ast::Expression* expr) {
}
info.source_type = TypeOf(source);
// If our initial access is into a non-pointer array, and either has a
// non-scalar element type or the accessor uses a non-literal index, then we
// need to load that array into a variable in order to access chain into it.
// TODO(bclayton): The requirement for scalar element types is because of
// arrays-of-arrays - this logic only considers whether the root index is
// compile-time-constant, and not whether there are any dynamic, inner-array
// indexing being performed. Instead of trying to do complex hoisting in this
// writer, move this hoisting into the transform::Spirv sanitizer.
bool needs_load = false; // Was the expression hoist to a temporary variable?
if (auto* access = accessors[0]->As<ast::ArrayAccessorExpression>()) {
auto* array = TypeOf(access->array())->As<sem::Array>();
bool trivial_indexing =
array && array->ElemType()->is_scalar() &&
access->idx_expr()->Is<ast::ScalarConstructorExpression>();
if (array && !trivial_indexing) {
// Wrap the source type in a reference to function storage.
auto* ref =
builder_.create<sem::Reference>(array, ast::StorageClass::kFunction);
auto result_type_id = GenerateTypeIfNeeded(ref);
if (result_type_id == 0) {
return 0;
}
auto ary_result = result_op();
auto init = GenerateConstantNullIfNeeded(array);
// If we're access chaining into an array then we must be in a function
push_function_var(
{Operand::Int(result_type_id), ary_result,
Operand::Int(ConvertStorageClass(ast::StorageClass::kFunction)),
Operand::Int(init)});
if (!push_function_inst(spv::Op::OpStore,
{ary_result, Operand::Int(info.source_id)})) {
return false;
}
info.source_id = ary_result.to_i();
info.source_type = ref;
needs_load = true;
bool literal_index =
array && access->idx_expr()->Is<ast::ScalarConstructorExpression>();
if (array && !literal_index) {
TINT_ICE(builder_.Diagnostics())
<< "Dynamic index on array value should have been promoted to "
"storage with the VarForDynamicIndex transform";
}
}
std::vector<uint32_t> access_chain_indices;
for (auto* accessor : accessors) {
if (auto* array = accessor->As<ast::ArrayAccessorExpression>()) {
if (!GenerateArrayAccessor(array, &info)) {
@@ -1138,10 +1102,6 @@ uint32_t Builder::GenerateAccessorExpression(ast::Expression* expr) {
if (!info.access_chain_indices.empty()) {
auto* type = TypeOf(expr);
if (needs_load) {
type =
builder_.create<sem::Reference>(type, ast::StorageClass::kFunction);
}
auto result_type_id = GenerateTypeIfNeeded(type);
if (result_type_id == 0) {
return 0;
@@ -1160,11 +1120,6 @@ uint32_t Builder::GenerateAccessorExpression(ast::Expression* expr) {
return false;
}
info.source_id = result_id;
// Load from the access chain result if required.
if (needs_load) {
info.source_id = GenerateLoadIfNeeded(type, result_id);
}
}
return info.source_id;

View File

@@ -762,37 +762,32 @@ TEST_F(BuilderTest, Accessor_Array_Of_Vec) {
auto* expr = IndexAccessor("pos", 1u);
WrapInFunction(var, expr);
spirv::Builder& b = Build();
spirv::Builder& b = SanitizeAndBuild();
b.push_function(Function{});
ASSERT_TRUE(b.GenerateFunctionVariable(var)) << b.error();
EXPECT_EQ(b.GenerateAccessorExpression(expr), 19u) << b.error();
ASSERT_TRUE(b.Build());
EXPECT_EQ(DumpInstructions(b.types()), R"(%3 = OpTypeFloat 32
%2 = OpTypeVector %3 2
%4 = OpTypeInt 32 0
%5 = OpConstant %4 3
%1 = OpTypeArray %2 %5
%6 = OpConstant %3 0
%7 = OpConstant %3 0.5
%8 = OpConstantComposite %2 %6 %7
%9 = OpConstant %3 -0.5
%10 = OpConstantComposite %2 %9 %9
%11 = OpConstantComposite %2 %7 %9
%12 = OpConstantComposite %1 %8 %10 %11
%13 = OpTypePointer Function %1
%15 = OpConstantNull %1
%16 = OpConstant %4 1
%17 = OpTypePointer Function %2
)");
EXPECT_EQ(DumpInstructions(b.functions()[0].variables()),
R"(%14 = OpVariable %13 Function %15
EXPECT_EQ(DumpInstructions(b.types()), R"(%2 = OpTypeVoid
%1 = OpTypeFunction %2
%7 = OpTypeFloat 32
%6 = OpTypeVector %7 2
%8 = OpTypeInt 32 0
%9 = OpConstant %8 3
%5 = OpTypeArray %6 %9
%10 = OpConstant %7 0
%11 = OpConstant %7 0.5
%12 = OpConstantComposite %6 %10 %11
%13 = OpConstant %7 -0.5
%14 = OpConstantComposite %6 %13 %13
%15 = OpConstantComposite %6 %11 %13
%16 = OpConstantComposite %5 %12 %14 %15
%17 = OpConstant %8 1
)");
EXPECT_EQ(DumpInstructions(b.functions()[0].variables()), R"()");
EXPECT_EQ(DumpInstructions(b.functions()[0].instructions()),
R"(OpStore %14 %12
%18 = OpAccessChain %17 %14 %16
%19 = OpLoad %2 %18
R"(%18 = OpCompositeExtract %6 %16 1
)");
Validate(b);
}
TEST_F(BuilderTest, Accessor_Array_Of_Array_Of_f32) {
@@ -810,39 +805,34 @@ TEST_F(BuilderTest, Accessor_Array_Of_Array_Of_f32) {
auto* expr = IndexAccessor(IndexAccessor("pos", 2u), 1u);
WrapInFunction(var, expr);
spirv::Builder& b = Build();
spirv::Builder& b = SanitizeAndBuild();
b.push_function(Function{});
ASSERT_TRUE(b.GenerateFunctionVariable(var)) << b.error();
EXPECT_EQ(b.GenerateAccessorExpression(expr), 21u) << b.error();
ASSERT_TRUE(b.Build());
EXPECT_EQ(DumpInstructions(b.types()), R"(%3 = OpTypeFloat 32
%2 = OpTypeVector %3 2
%4 = OpTypeInt 32 0
%5 = OpConstant %4 3
%1 = OpTypeArray %2 %5
%6 = OpConstant %3 0
%7 = OpConstant %3 0.5
%8 = OpConstantComposite %2 %6 %7
%9 = OpConstant %3 -0.5
%10 = OpConstantComposite %2 %9 %9
%11 = OpConstantComposite %2 %7 %9
%12 = OpConstantComposite %1 %8 %10 %11
%13 = OpTypePointer Function %1
%15 = OpConstantNull %1
%16 = OpConstant %4 2
%17 = OpConstant %4 1
%19 = OpTypePointer Function %3
)");
EXPECT_EQ(DumpInstructions(b.functions()[0].variables()),
R"(%14 = OpVariable %13 Function %15
EXPECT_EQ(DumpInstructions(b.types()), R"(%2 = OpTypeVoid
%1 = OpTypeFunction %2
%7 = OpTypeFloat 32
%6 = OpTypeVector %7 2
%8 = OpTypeInt 32 0
%9 = OpConstant %8 3
%5 = OpTypeArray %6 %9
%10 = OpConstant %7 0
%11 = OpConstant %7 0.5
%12 = OpConstantComposite %6 %10 %11
%13 = OpConstant %7 -0.5
%14 = OpConstantComposite %6 %13 %13
%15 = OpConstantComposite %6 %11 %13
%16 = OpConstantComposite %5 %12 %14 %15
%17 = OpConstant %8 2
%19 = OpConstant %8 1
)");
EXPECT_EQ(DumpInstructions(b.functions()[0].variables()), R"()");
EXPECT_EQ(DumpInstructions(b.functions()[0].instructions()),
R"(OpStore %14 %12
%18 = OpCompositeExtract %3 %14 1
%20 = OpAccessChain %19 %18 %16
%21 = OpLoad %3 %20
R"(%18 = OpCompositeExtract %6 %16 2
%20 = OpCompositeExtract %7 %18 1
)");
Validate(b);
}
TEST_F(BuilderTest, Accessor_Const_Vec) {
@@ -919,27 +909,29 @@ TEST_F(BuilderTest, Accessor_Array_NonPointer) {
auto* expr = IndexAccessor("a", 2);
WrapInFunction(var, expr);
spirv::Builder& b = Build();
spirv::Builder& b = SanitizeAndBuild();
b.push_function(Function{});
ASSERT_TRUE(b.GenerateGlobalVariable(var)) << b.error();
EXPECT_EQ(b.GenerateAccessorExpression(expr), 11u) << b.error();
ASSERT_TRUE(b.Build());
EXPECT_EQ(DumpInstructions(b.types()), R"(%2 = OpTypeFloat 32
%3 = OpTypeInt 32 0
%4 = OpConstant %3 3
%1 = OpTypeArray %2 %4
%5 = OpConstant %2 0
%6 = OpConstant %2 0.5
%7 = OpConstant %2 1
%8 = OpConstantComposite %1 %5 %6 %7
%9 = OpTypeInt 32 1
%10 = OpConstant %9 2
EXPECT_EQ(DumpInstructions(b.types()), R"(%2 = OpTypeVoid
%1 = OpTypeFunction %2
%6 = OpTypeFloat 32
%7 = OpTypeInt 32 0
%8 = OpConstant %7 3
%5 = OpTypeArray %6 %8
%9 = OpConstant %6 0
%10 = OpConstant %6 0.5
%11 = OpConstant %6 1
%12 = OpConstantComposite %5 %9 %10 %11
%13 = OpTypeInt 32 1
%14 = OpConstant %13 2
)");
EXPECT_EQ(DumpInstructions(b.functions()[0].variables()), "");
EXPECT_EQ(DumpInstructions(b.functions()[0].instructions()),
R"(%11 = OpCompositeExtract %2 %8 2
R"(%15 = OpCompositeExtract %6 %12 2
)");
Validate(b);
}
TEST_F(BuilderTest, Accessor_Array_NonPointer_Dynamic) {
@@ -955,38 +947,39 @@ TEST_F(BuilderTest, Accessor_Array_NonPointer_Dynamic) {
WrapInFunction(var, idx, expr);
spirv::Builder& b = Build();
spirv::Builder& b = SanitizeAndBuild();
b.push_function(Function{});
ASSERT_TRUE(b.GenerateGlobalVariable(var)) << b.error();
ASSERT_TRUE(b.GenerateFunctionVariable(idx)) << b.error();
EXPECT_EQ(b.GenerateAccessorExpression(expr), 19u) << b.error();
ASSERT_TRUE(b.Build());
EXPECT_EQ(DumpInstructions(b.types()), R"(%2 = OpTypeFloat 32
%3 = OpTypeInt 32 0
%4 = OpConstant %3 3
%1 = OpTypeArray %2 %4
%5 = OpConstant %2 0
%6 = OpConstant %2 0.5
%7 = OpConstant %2 1
%8 = OpConstantComposite %1 %5 %6 %7
%11 = OpTypeInt 32 1
%10 = OpTypePointer Function %11
%12 = OpConstantNull %11
%13 = OpTypePointer Function %1
%15 = OpConstantNull %1
%17 = OpTypePointer Function %2
EXPECT_EQ(DumpInstructions(b.types()), R"(%2 = OpTypeVoid
%1 = OpTypeFunction %2
%6 = OpTypeFloat 32
%7 = OpTypeInt 32 0
%8 = OpConstant %7 3
%5 = OpTypeArray %6 %8
%9 = OpConstant %6 0
%10 = OpConstant %6 0.5
%11 = OpConstant %6 1
%12 = OpConstantComposite %5 %9 %10 %11
%15 = OpTypeInt 32 1
%14 = OpTypePointer Function %15
%16 = OpConstantNull %15
%18 = OpTypePointer Function %5
%19 = OpConstantNull %5
%21 = OpTypePointer Function %6
)");
EXPECT_EQ(DumpInstructions(b.functions()[0].variables()),
R"(%9 = OpVariable %10 Function %12
%14 = OpVariable %13 Function %15
R"(%13 = OpVariable %14 Function %16
%17 = OpVariable %18 Function %19
)");
EXPECT_EQ(DumpInstructions(b.functions()[0].instructions()),
R"(OpStore %14 %8
%16 = OpLoad %11 %9
%18 = OpAccessChain %17 %14 %16
%19 = OpLoad %2 %18
R"(OpStore %17 %12
%20 = OpLoad %15 %13
%22 = OpAccessChain %21 %17 %20
%23 = OpLoad %6 %22
)");
Validate(b);
}
} // namespace