From 8e38fad091fac4e2970e0cbe2bcb11302903b4d0 Mon Sep 17 00:00:00 2001 From: Ben Clayton Date: Thu, 15 Jul 2021 22:20:29 +0000 Subject: [PATCH] transform/InlinePtrLets: Fix ICE for lets in for-loops For loop initializers and continuing statements do not have a BlockStatement as their parent. Handle removal of these statements with a new Transform::RemoveStatement() helper Fixed: tint:990 Change-Id: I24e7b18dcf71d3ef0a4d3ee68b9f68518e0eb5e8 Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/58063 Kokoro: Kokoro Reviewed-by: James Price --- src/clone_context.h | 7 ++++-- src/transform/inline_pointer_lets.cc | 2 +- src/transform/transform.cc | 17 +++++++++++++ src/transform/transform.h | 7 ++++++ test/bug/tint/990.wgsl | 4 +++ test/bug/tint/990.wgsl.expected.hlsl | 12 +++++++++ test/bug/tint/990.wgsl.expected.msl | 9 +++++++ test/bug/tint/990.wgsl.expected.spvasm | 35 ++++++++++++++++++++++++++ test/bug/tint/990.wgsl.expected.wgsl | 5 ++++ 9 files changed, 95 insertions(+), 3 deletions(-) create mode 100644 test/bug/tint/990.wgsl create mode 100644 test/bug/tint/990.wgsl.expected.hlsl create mode 100644 test/bug/tint/990.wgsl.expected.msl create mode 100644 test/bug/tint/990.wgsl.expected.spvasm create mode 100644 test/bug/tint/990.wgsl.expected.wgsl diff --git a/src/clone_context.h b/src/clone_context.h index 8be0b9f196..c5437a59b7 100644 --- a/src/clone_context.h +++ b/src/clone_context.h @@ -513,12 +513,15 @@ class CloneContext { /// Reports an internal compiler error if the cast failed. template TO* CheckedCast(FROM* obj) { - if (TO* cast = As(obj)) { + if (obj == nullptr) { + return nullptr; + } + if (TO* cast = obj->template As()) { return cast; } TINT_ICE(Clone, Diagnostics()) << "Cloned object was not of the expected type\n" - << "got: " << (obj ? obj->TypeInfo().name : "") << "\n" + << "got: " << obj->TypeInfo().name << "\n" << "expected: " << TypeInfo::Of().name; return nullptr; } diff --git a/src/transform/inline_pointer_lets.cc b/src/transform/inline_pointer_lets.cc index 6b18f75b47..38d905c916 100644 --- a/src/transform/inline_pointer_lets.cc +++ b/src/transform/inline_pointer_lets.cc @@ -177,7 +177,7 @@ void InlinePointerLets::Run(CloneContext& ctx, const DataMap&, DataMap&) { ptr_lets.emplace(let->variable(), std::move(ptr_let)); // As the original `let` declaration will be fully inlined, there's no // need for the original declaration to exist. Remove it. - ctx.Remove(block->statements(), let); + RemoveStatement(ctx, let); } } diff --git a/src/transform/transform.cc b/src/transform/transform.cc index 8342d1f355..cd788c5eae 100644 --- a/src/transform/transform.cc +++ b/src/transform/transform.cc @@ -19,6 +19,8 @@ #include "src/program_builder.h" #include "src/sem/atomic_type.h" +#include "src/sem/block_statement.h" +#include "src/sem/for_loop_statement.h" #include "src/sem/reference_type.h" TINT_INSTANTIATE_TYPEINFO(tint::transform::Transform); @@ -84,6 +86,21 @@ ast::DecorationList Transform::RemoveDecorations( return new_decorations; } +void Transform::RemoveStatement(CloneContext& ctx, ast::Statement* stmt) { + auto* sem = ctx.src->Sem().Get(stmt); + if (auto* block = tint::As(sem->Parent())) { + ctx.Remove(block->Declaration()->statements(), stmt); + return; + } + if (tint::Is(sem->Parent())) { + ctx.Replace(stmt, static_cast(nullptr)); + return; + } + TINT_ICE(Transform, ctx.dst->Diagnostics()) + << "unable to remove statement from parent of type " + << sem->TypeInfo().name; +} + ast::Type* Transform::CreateASTTypeFor(CloneContext& ctx, const sem::Type* ty) { if (ty->Is()) { return ctx.dst->create(); diff --git a/src/transform/transform.h b/src/transform/transform.h index 08bf9459d0..9f9e6d3aa5 100644 --- a/src/transform/transform.h +++ b/src/transform/transform.h @@ -195,6 +195,13 @@ class Transform : public Castable { const ast::DecorationList& in, std::function should_remove); + /// Removes the statement `stmt` from the transformed program. + /// RemoveStatement handles edge cases, like statements in the initializer and + /// continuing of for-loops. + /// @param ctx the clone context + /// @param stmt the statement to remove when the program is cloned + static void RemoveStatement(CloneContext& ctx, ast::Statement* stmt); + /// CreateASTTypeFor constructs new ast::Type nodes that reconstructs the /// semantic type `ty`. /// @param ctx the clone context diff --git a/test/bug/tint/990.wgsl b/test/bug/tint/990.wgsl new file mode 100644 index 0000000000..3f31b567e3 --- /dev/null +++ b/test/bug/tint/990.wgsl @@ -0,0 +1,4 @@ +fn f() { + var i : i32; + for (let p = &i;;) {} +} diff --git a/test/bug/tint/990.wgsl.expected.hlsl b/test/bug/tint/990.wgsl.expected.hlsl new file mode 100644 index 0000000000..80751f801d --- /dev/null +++ b/test/bug/tint/990.wgsl.expected.hlsl @@ -0,0 +1,12 @@ +[numthreads(1, 1, 1)] +void unused_entry_point() { + return; +} + +void f() { + int i = 0; + { + for(; ; ) { + } + } +} diff --git a/test/bug/tint/990.wgsl.expected.msl b/test/bug/tint/990.wgsl.expected.msl new file mode 100644 index 0000000000..62e37cec3b --- /dev/null +++ b/test/bug/tint/990.wgsl.expected.msl @@ -0,0 +1,9 @@ +#include + +using namespace metal; +void f() { + int i = 0; + for(; ; ) { + } +} + diff --git a/test/bug/tint/990.wgsl.expected.spvasm b/test/bug/tint/990.wgsl.expected.spvasm new file mode 100644 index 0000000000..a63370b31b --- /dev/null +++ b/test/bug/tint/990.wgsl.expected.spvasm @@ -0,0 +1,35 @@ +; SPIR-V +; Version: 1.3 +; Generator: Google Tint Compiler; 0 +; Bound: 15 +; Schema: 0 + OpCapability Shader + OpMemoryModel Logical GLSL450 + OpEntryPoint GLCompute %unused_entry_point "unused_entry_point" + OpExecutionMode %unused_entry_point LocalSize 1 1 1 + OpName %unused_entry_point "unused_entry_point" + OpName %f "f" + OpName %i "i" + %void = OpTypeVoid + %1 = OpTypeFunction %void + %int = OpTypeInt 32 1 +%_ptr_Function_int = OpTypePointer Function %int + %10 = OpConstantNull %int +%unused_entry_point = OpFunction %void None %1 + %4 = OpLabel + OpReturn + OpFunctionEnd + %f = OpFunction %void None %1 + %6 = OpLabel + %i = OpVariable %_ptr_Function_int Function %10 + OpBranch %11 + %11 = OpLabel + OpLoopMerge %12 %13 None + OpBranch %14 + %14 = OpLabel + OpBranch %13 + %13 = OpLabel + OpBranch %11 + %12 = OpLabel + OpReturn + OpFunctionEnd diff --git a/test/bug/tint/990.wgsl.expected.wgsl b/test/bug/tint/990.wgsl.expected.wgsl new file mode 100644 index 0000000000..e7341b6336 --- /dev/null +++ b/test/bug/tint/990.wgsl.expected.wgsl @@ -0,0 +1,5 @@ +fn f() { + var i : i32; + for(let p = &(i); ; ;) { + } +}