From 51e37c6f91cfbe2be77c470027f71a0d06992212 Mon Sep 17 00:00:00 2001 From: Ben Clayton Date: Fri, 14 Jan 2022 10:16:24 +0000 Subject: [PATCH] Fixes for bugs around unreachable code Remove the ICE check for expression behaviors always being either `{Next}` or `{Next, Discard}`. Unreachable code may be result in something else. Add the RemoveUnreachableStatements transform to the SPIR-V writer sanitizer transform list. The writer cannot correctly handle unreachable statements. Bug: tint:1369 Bug: chromium:1285622 Change-Id: I9fa54c6d2096b1ee633dd551b628c7dd3ba64fb5 Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/76300 Kokoro: Kokoro Reviewed-by: David Neto Commit-Queue: Ben Clayton --- src/resolver/resolver.cc | 12 -------- src/writer/spirv/builder.cc | 2 ++ test/bug/tint/1369.wgsl | 10 +++++++ test/bug/tint/1369.wgsl.expected.hlsl | 22 +++++++++++++++ test/bug/tint/1369.wgsl.expected.msl | 22 +++++++++++++++ test/bug/tint/1369.wgsl.expected.spvasm | 37 +++++++++++++++++++++++++ test/bug/tint/1369.wgsl.expected.wgsl | 18 ++++++++++++ 7 files changed, 111 insertions(+), 12 deletions(-) create mode 100644 test/bug/tint/1369.wgsl create mode 100644 test/bug/tint/1369.wgsl.expected.hlsl create mode 100644 test/bug/tint/1369.wgsl.expected.msl create mode 100644 test/bug/tint/1369.wgsl.expected.spvasm create mode 100644 test/bug/tint/1369.wgsl.expected.wgsl diff --git a/src/resolver/resolver.cc b/src/resolver/resolver.cc index e346ed33f9..d732f0c09c 100644 --- a/src/resolver/resolver.cc +++ b/src/resolver/resolver.cc @@ -1149,18 +1149,6 @@ sem::Expression* Resolver::Expression(const ast::Expression* root) { return nullptr; } - // https://www.w3.org/TR/WGSL/#behaviors-rules - // an expression behavior is always either {Next} or {Next, Discard} - if (sem_expr->Behaviors() != sem::Behavior::kNext && - sem_expr->Behaviors() != sem::Behaviors{sem::Behavior::kNext, // NOLINT - sem::Behavior::kDiscard} && - !IsCallStatement(expr)) { - TINT_ICE(Resolver, diagnostics_) - << expr->TypeInfo().name - << " behaviors are: " << sem_expr->Behaviors(); - return nullptr; - } - builder_->Sem().Add(expr, sem_expr); if (expr == root) { return sem_expr; diff --git a/src/writer/spirv/builder.cc b/src/writer/spirv/builder.cc index f7f354f1c9..f4b93acced 100644 --- a/src/writer/spirv/builder.cc +++ b/src/writer/spirv/builder.cc @@ -47,6 +47,7 @@ #include "src/transform/fold_constants.h" #include "src/transform/for_loop_to_loop.h" #include "src/transform/manager.h" +#include "src/transform/remove_unreachable_statements.h" #include "src/transform/simplify_pointers.h" #include "src/transform/unshadow.h" #include "src/transform/var_for_dynamic_index.h" @@ -260,6 +261,7 @@ SanitizedResult Sanitize(const Program* in, if (!disable_workgroup_init) { manager.Add(); } + manager.Add(); manager.Add(); // Required for arrayLength() manager.Add(); manager.Add(); diff --git a/test/bug/tint/1369.wgsl b/test/bug/tint/1369.wgsl new file mode 100644 index 0000000000..434bb2b37f --- /dev/null +++ b/test/bug/tint/1369.wgsl @@ -0,0 +1,10 @@ +fn call_discard() -> bool { + discard; + return true; +} + +[[stage(fragment)]] +fn f() { + var v = call_discard(); + var also_unreachable : bool; +} diff --git a/test/bug/tint/1369.wgsl.expected.hlsl b/test/bug/tint/1369.wgsl.expected.hlsl new file mode 100644 index 0000000000..dff55b391a --- /dev/null +++ b/test/bug/tint/1369.wgsl.expected.hlsl @@ -0,0 +1,22 @@ +bug/tint/1369.wgsl:3:3 warning: code is unreachable + return true; + ^^^^^^ + +bug/tint/1369.wgsl:9:9 warning: code is unreachable + var also_unreachable : bool; + ^^^^^^^^^^^^^^^^ + +bool call_discard() { + if (true) { + discard; + return true; + } + bool unused; + return unused; +} + +void f() { + bool v = call_discard(); + bool also_unreachable = false; + return; +} diff --git a/test/bug/tint/1369.wgsl.expected.msl b/test/bug/tint/1369.wgsl.expected.msl new file mode 100644 index 0000000000..8d1a7d7685 --- /dev/null +++ b/test/bug/tint/1369.wgsl.expected.msl @@ -0,0 +1,22 @@ +bug/tint/1369.wgsl:3:3 warning: code is unreachable + return true; + ^^^^^^ + +bug/tint/1369.wgsl:9:9 warning: code is unreachable + var also_unreachable : bool; + ^^^^^^^^^^^^^^^^ + +#include + +using namespace metal; +bool call_discard() { + discard_fragment(); + return true; +} + +fragment void f() { + bool v = call_discard(); + bool also_unreachable = false; + return; +} + diff --git a/test/bug/tint/1369.wgsl.expected.spvasm b/test/bug/tint/1369.wgsl.expected.spvasm new file mode 100644 index 0000000000..a78ae04764 --- /dev/null +++ b/test/bug/tint/1369.wgsl.expected.spvasm @@ -0,0 +1,37 @@ +bug/tint/1369.wgsl:3:3 warning: code is unreachable + return true; + ^^^^^^ + +bug/tint/1369.wgsl:9:9 warning: code is unreachable + var also_unreachable : bool; + ^^^^^^^^^^^^^^^^ + +; SPIR-V +; Version: 1.3 +; Generator: Google Tint Compiler; 0 +; Bound: 13 +; Schema: 0 + OpCapability Shader + OpMemoryModel Logical GLSL450 + OpEntryPoint Fragment %f "f" + OpExecutionMode %f OriginUpperLeft + OpName %call_discard "call_discard" + OpName %f "f" + OpName %v "v" + %bool = OpTypeBool + %1 = OpTypeFunction %bool + %void = OpTypeVoid + %5 = OpTypeFunction %void +%_ptr_Function_bool = OpTypePointer Function %bool + %12 = OpConstantNull %bool +%call_discard = OpFunction %bool None %1 + %4 = OpLabel + OpKill + OpFunctionEnd + %f = OpFunction %void None %5 + %8 = OpLabel + %v = OpVariable %_ptr_Function_bool Function %12 + %9 = OpFunctionCall %bool %call_discard + OpStore %v %9 + OpReturn + OpFunctionEnd diff --git a/test/bug/tint/1369.wgsl.expected.wgsl b/test/bug/tint/1369.wgsl.expected.wgsl new file mode 100644 index 0000000000..eb44227d81 --- /dev/null +++ b/test/bug/tint/1369.wgsl.expected.wgsl @@ -0,0 +1,18 @@ +bug/tint/1369.wgsl:3:3 warning: code is unreachable + return true; + ^^^^^^ + +bug/tint/1369.wgsl:9:9 warning: code is unreachable + var also_unreachable : bool; + ^^^^^^^^^^^^^^^^ + +fn call_discard() -> bool { + discard; + return true; +} + +[[stage(fragment)]] +fn f() { + var v = call_discard(); + var also_unreachable : bool; +}