Fix overrides in array size.

This CL fixes the usage of overrides in array sizes. Currently
the usage will generate a validation error as we check that the
array size is const.

Bug: tint:1660
Change-Id: Ibf440905c30a73b581d55b0c071b8621b61605e6
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/101900
Kokoro: Kokoro <noreply+kokoro@google.com>
Reviewed-by: dan sinclair <dsinclair@google.com>
Reviewed-by: Antonio Maiorano <amaiorano@google.com>
Commit-Queue: Ben Clayton <bclayton@chromium.org>
This commit is contained in:
dan sinclair
2022-09-22 22:28:21 +00:00
committed by Dawn LUCI CQ
parent 534a198f88
commit 78f8067fd5
42 changed files with 839 additions and 186 deletions

View File

@@ -471,8 +471,18 @@ struct DecomposeMemoryAccess::State {
auto* arr = b.Var(b.Symbols().New("arr"), CreateASTTypeFor(ctx, arr_ty));
auto* i = b.Var(b.Symbols().New("i"), b.Expr(0_u));
auto* for_init = b.Decl(i);
auto arr_cnt = arr_ty->ConstantCount();
if (!arr_cnt) {
// Non-constant counts should not be possible:
// * Override-expression counts can only be applied to workgroup arrays, and
// this method only handles storage and uniform.
// * Runtime-sized arrays are not loadable.
TINT_ICE(Transform, ctx.dst->Diagnostics())
<< "unexpected non-constant array count";
arr_cnt = 1;
}
auto* for_cond = b.create<ast::BinaryExpression>(
ast::BinaryOp::kLessThan, b.Expr(i), b.Expr(u32(arr_ty->Count())));
ast::BinaryOp::kLessThan, b.Expr(i), b.Expr(u32(arr_cnt.value())));
auto* for_cont = b.Assign(i, b.Add(i, 1_u));
auto* arr_el = b.IndexAccessor(arr, i);
auto* el_offset = b.Add(b.Expr("offset"), b.Mul(i, u32(arr_ty->Stride())));
@@ -562,8 +572,18 @@ struct DecomposeMemoryAccess::State {
StoreFunc(buf_ty, arr_ty->ElemType()->UnwrapRef(), var_user);
auto* i = b.Var(b.Symbols().New("i"), b.Expr(0_u));
auto* for_init = b.Decl(i);
auto arr_cnt = arr_ty->ConstantCount();
if (!arr_cnt) {
// Non-constant counts should not be possible:
// * Override-expression counts can only be applied to workgroup
// arrays, and this method only handles storage and uniform.
// * Runtime-sized arrays are not storable.
TINT_ICE(Transform, ctx.dst->Diagnostics())
<< "unexpected non-constant array count";
arr_cnt = 1;
}
auto* for_cond = b.create<ast::BinaryExpression>(
ast::BinaryOp::kLessThan, b.Expr(i), b.Expr(u32(arr_ty->Count())));
ast::BinaryOp::kLessThan, b.Expr(i), b.Expr(u32(arr_cnt.value())));
auto* for_cont = b.Assign(i, b.Add(i, 1_u));
auto* arr_el = b.IndexAccessor(array, i);
auto* el_offset =

View File

@@ -82,7 +82,7 @@ void PadStructs::Run(CloneContext& ctx, const DataMap&, DataMap&) const {
// std140 structs should be padded out to 16 bytes.
size = utils::RoundUp(16u, size);
} else if (auto* array_ty = ty->As<sem::Array>()) {
if (array_ty->Count() == 0) {
if (array_ty->IsRuntimeSized()) {
has_runtime_sized_array = true;
}
}

View File

@@ -99,14 +99,21 @@ struct Robustness::State {
// Must clamp, even if the index is constant.
auto* arr_ptr = b.AddressOf(ctx.Clone(expr->object));
max = b.Sub(b.Call("arrayLength", arr_ptr), 1_u);
} else {
} else if (auto count = arr->ConstantCount()) {
if (sem->Index()->ConstantValue()) {
// Index and size is constant.
// Validation will have rejected any OOB accesses.
return nullptr;
}
max = b.Expr(u32(arr->Count() - 1u));
max = b.Expr(u32(count.value() - 1u));
} else {
// Note: Don't be tempted to use the array override variable as an expression
// here, the name might be shadowed!
ctx.dst->Diagnostics().add_error(diag::System::Transform,
sem::Array::kErrExpectedConstantCount);
return nullptr;
}
return b.Call("min", idx(), max);
},
[&](Default) {

View File

@@ -1316,5 +1316,23 @@ fn f() {
EXPECT_EQ(expect, str(got));
}
TEST_F(RobustnessTest, WorkgroupOverrideCount) {
auto* src = R"(
override N = 123;
var<workgroup> w : array<f32, N>;
fn f() {
var b : f32 = w[1i];
}
)";
auto* expect = R"(error: array size is an override-expression, when expected a constant-expression.
Was the SubstituteOverride transform run?)";
auto got = Run<Robustness>(src);
EXPECT_EQ(expect, str(got));
}
} // namespace
} // namespace tint::transform

View File

@@ -35,6 +35,8 @@ TINT_INSTANTIATE_TYPEINFO(tint::transform::SpirvAtomic::Stub);
namespace tint::transform {
using namespace tint::number_suffixes; // NOLINT
/// Private implementation of transform
struct SpirvAtomic::State {
private:
@@ -189,10 +191,19 @@ struct SpirvAtomic::State {
[&](const sem::I32*) { return b.ty.atomic(CreateASTTypeFor(ctx, ty)); },
[&](const sem::U32*) { return b.ty.atomic(CreateASTTypeFor(ctx, ty)); },
[&](const sem::Struct* str) { return b.ty.type_name(Fork(str->Declaration()).name); },
[&](const sem::Array* arr) {
return arr->IsRuntimeSized()
? b.ty.array(AtomicTypeFor(arr->ElemType()))
: b.ty.array(AtomicTypeFor(arr->ElemType()), u32(arr->Count()));
[&](const sem::Array* arr) -> const ast::Type* {
if (arr->IsRuntimeSized()) {
return b.ty.array(AtomicTypeFor(arr->ElemType()));
}
auto count = arr->ConstantCount();
if (!count) {
ctx.dst->Diagnostics().add_error(
diag::System::Transform,
"the SpirvAtomic transform does not currently support array counts that "
"use override values");
count = 1;
}
return b.ty.array(AtomicTypeFor(arr->ElemType()), u32(count.value()));
},
[&](const sem::Pointer* ptr) {
return b.ty.pointer(AtomicTypeFor(ptr->StoreType()), ptr->StorageClass(),

View File

@@ -423,7 +423,17 @@ struct Std140::State {
if (!arr->IsStrideImplicit()) {
attrs.Push(ctx.dst->create<ast::StrideAttribute>(arr->Stride()));
}
return b.create<ast::Array>(std140, b.Expr(u32(arr->Count())),
auto count = arr->ConstantCount();
if (!count) {
// Non-constant counts should not be possible:
// * Override-expression counts can only be applied to workgroup arrays, and
// this method only handles types transitively used as uniform buffers.
// * Runtime-sized arrays cannot be used in uniform buffers.
TINT_ICE(Transform, ctx.dst->Diagnostics())
<< "unexpected non-constant array count";
count = 1;
}
return b.create<ast::Array>(std140, b.Expr(u32(count.value())),
std::move(attrs));
}
return nullptr;
@@ -613,7 +623,17 @@ struct Std140::State {
ty, //
[&](const sem::Struct* str) { return sym.NameFor(str->Name()); },
[&](const sem::Array* arr) {
return "arr" + std::to_string(arr->Count()) + "_" + ConvertSuffix(arr->ElemType());
auto count = arr->ConstantCount();
if (!count) {
// Non-constant counts should not be possible:
// * Override-expression counts can only be applied to workgroup arrays, and
// this method only handles types transitively used as uniform buffers.
// * Runtime-sized arrays cannot be used in uniform buffers.
TINT_ICE(Transform, ctx.dst->Diagnostics())
<< "unexpected non-constant array count";
count = 1;
}
return "arr" + std::to_string(count.value()) + "_" + ConvertSuffix(arr->ElemType());
},
[&](const sem::Matrix* mat) {
return "mat" + std::to_string(mat->columns()) + "x" + std::to_string(mat->rows()) +
@@ -710,10 +730,20 @@ struct Std140::State {
auto* i = b.Var("i", b.ty.u32());
auto* dst_el = b.IndexAccessor(var, i);
auto* src_el = Convert(arr->ElemType(), b.IndexAccessor(param, i));
auto count = arr->ConstantCount();
if (!count) {
// Non-constant counts should not be possible:
// * Override-expression counts can only be applied to workgroup arrays, and
// this method only handles types transitively used as uniform buffers.
// * Runtime-sized arrays cannot be used in uniform buffers.
TINT_ICE(Transform, ctx.dst->Diagnostics())
<< "unexpected non-constant array count";
count = 1;
}
stmts.Push(b.Decl(var));
stmts.Push(b.For(b.Decl(i), //
b.LessThan(i, u32(arr->Count())), //
b.Assign(i, b.Add(i, 1_a)), //
stmts.Push(b.For(b.Decl(i), //
b.LessThan(i, u32(count.value())), //
b.Assign(i, b.Add(i, 1_a)), //
b.Block(b.Assign(dst_el, src_el))));
stmts.Push(b.Return(var));
},

View File

@@ -24,6 +24,7 @@
#include "src/tint/sem/for_loop_statement.h"
#include "src/tint/sem/reference.h"
#include "src/tint/sem/sampler.h"
#include "src/tint/sem/variable.h"
TINT_INSTANTIATE_TYPEINFO(tint::transform::Transform);
TINT_INSTANTIATE_TYPEINFO(tint::transform::Data);
@@ -112,9 +113,16 @@ const ast::Type* Transform::CreateASTTypeFor(CloneContext& ctx, const sem::Type*
}
if (a->IsRuntimeSized()) {
return ctx.dst->ty.array(el, nullptr, std::move(attrs));
} else {
return ctx.dst->ty.array(el, u32(a->Count()), std::move(attrs));
}
if (auto* override = std::get_if<sem::OverrideArrayCount>(&a->Count())) {
auto* count = ctx.Clone(override->variable->Declaration());
return ctx.dst->ty.array(el, count, std::move(attrs));
}
if (auto count = a->ConstantCount()) {
return ctx.dst->ty.array(el, u32(count.value()), std::move(attrs));
}
TINT_ICE(Transform, ctx.dst->Diagnostics()) << sem::Array::kErrExpectedConstantCount;
return ctx.dst->ty.array(el, u32(1), std::move(attrs));
}
if (auto* s = ty->As<sem::Struct>()) {
return ctx.dst->create<ast::TypeName>(ctx.Clone(s->Declaration()->name));

View File

@@ -65,7 +65,8 @@ TEST_F(CreateASTTypeForTest, Vector) {
TEST_F(CreateASTTypeForTest, ArrayImplicitStride) {
auto* arr = create([](ProgramBuilder& b) {
return b.create<sem::Array>(b.create<sem::F32>(), 2u, 4u, 4u, 32u, 32u);
return b.create<sem::Array>(b.create<sem::F32>(), sem::ConstantArrayCount{2u}, 4u, 4u, 32u,
32u);
});
ASSERT_TRUE(arr->Is<ast::Array>());
ASSERT_TRUE(arr->As<ast::Array>()->type->Is<ast::F32>());
@@ -78,7 +79,8 @@ TEST_F(CreateASTTypeForTest, ArrayImplicitStride) {
TEST_F(CreateASTTypeForTest, ArrayNonImplicitStride) {
auto* arr = create([](ProgramBuilder& b) {
return b.create<sem::Array>(b.create<sem::F32>(), 2u, 4u, 4u, 64u, 32u);
return b.create<sem::Array>(b.create<sem::F32>(), sem::ConstantArrayCount{2u}, 4u, 4u, 64u,
32u);
});
ASSERT_TRUE(arr->Is<ast::Array>());
ASSERT_TRUE(arr->As<ast::Array>()->type->Is<ast::F32>());

View File

@@ -307,7 +307,13 @@ struct ZeroInitWorkgroupMemory::State {
// `num_values * arr->Count()`
// The index for this array is:
// `(idx % modulo) / division`
auto modulo = num_values * arr->Count();
auto count = arr->ConstantCount();
if (!count) {
ctx.dst->Diagnostics().add_error(diag::System::Transform,
sem::Array::kErrExpectedConstantCount);
return Expression{};
}
auto modulo = num_values * count.value();
auto division = num_values;
auto a = get_expr(modulo);
auto array_indices = a.array_indices;