tint: fix const eval short-circuiting with mixed runtime and constant expressions For logical binary expressions that can be short-circuited, if the rhs tree contained a mix of constant and runtime expressions, we would erroneously mark the node as runtime, although some of its children were resolved as kNotEvaluated. This would then fail during backend generation. This is a fork of 115820, addressing review comments, as amaiorano is OOO this week. Bug: chromium:1403752 Bug: tint:1581 Change-Id: I18682c7fe1db092d280390881ff86b3c0db23e9b Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/116020 Commit-Queue: Ben Clayton <bclayton@google.com> Reviewed-by: James Price <jrprice@google.com> Kokoro: Kokoro <noreply+kokoro@google.com> Auto-Submit: Ben Clayton <bclayton@google.com>
diff --git a/src/tint/ir/builder_impl_test.cc b/src/tint/ir/builder_impl_test.cc index 41a3f4c..ced23f6 100644 --- a/src/tint/ir/builder_impl_test.cc +++ b/src/tint/ir/builder_impl_test.cc
@@ -1697,8 +1697,8 @@ } TEST_F(IR_BuilderImplTest, EmitExpression_Binary_Compound) { - auto* expr = LogicalOr(LessThan(1_u, Add(Shr(3_u, 4_u), 9_u)), - GreaterThan(2.5_f, Div(6.7_f, Mul(2.3_f, 5.5_f)))); + auto* expr = LogicalAnd(LessThan(1_u, Add(Shr(3_u, 4_u), 9_u)), + GreaterThan(2.5_f, Div(6.7_f, Mul(2.3_f, 5.5_f)))); WrapInFunction(expr); auto& b = CreateBuilder(); @@ -1714,7 +1714,7 @@ %4 (f32) = 2.3 * 5.5 %5 (f32) = 6.7 / %4 (f32) %6 (bool) = 2.5 > %5 (f32) -%7 (bool) = %3 (bool) || %6 (bool) +%7 (bool) = %3 (bool) && %6 (bool) )"); }
diff --git a/src/tint/resolver/const_eval_binary_op_test.cc b/src/tint/resolver/const_eval_binary_op_test.cc index 7ea998a..55d22ba 100644 --- a/src/tint/resolver/const_eval_binary_op_test.cc +++ b/src/tint/resolver/const_eval_binary_op_test.cc
@@ -2209,6 +2209,32 @@ } //////////////////////////////////////////////// +// Short-Circuit Mixed Constant and Runtime +//////////////////////////////////////////////// + +TEST_F(ResolverConstEvalTest, ShortCircuit_And_MixedConstantAndRuntime) { + // var j : i32; + // let result = false && j < (0 - 8); + auto* j = Decl(Var("j", ty.i32())); + auto* binary = LogicalAnd(Expr(false), LessThan("j", Sub(0_a, 8_a))); + auto* result = Let("result", binary); + WrapInFunction(j, result); + EXPECT_TRUE(r()->Resolve()) << r()->error(); + ValidateAnd(Sem(), binary); +} + +TEST_F(ResolverConstEvalTest, ShortCircuit_Or_MixedConstantAndRuntime) { + // var j : i32; + // let result = true || j < (0 - 8); + auto* j = Decl(Var("j", ty.i32())); + auto* binary = LogicalOr(Expr(true), LessThan("j", Sub(0_a, 8_a))); + auto* result = Let("result", binary); + WrapInFunction(j, result); + EXPECT_TRUE(r()->Resolve()) << r()->error(); + ValidateOr(Sem(), binary); +} + +//////////////////////////////////////////////// // Short-Circuit Nested ////////////////////////////////////////////////
diff --git a/src/tint/resolver/intrinsic_table.cc b/src/tint/resolver/intrinsic_table.cc index 54c851d..0b1a6d8 100644 --- a/src/tint/resolver/intrinsic_table.cc +++ b/src/tint/resolver/intrinsic_table.cc
@@ -356,7 +356,7 @@ } bool match_fa(MatchState& state, const type::Type* ty) { - return (state.earliest_eval_stage == sem::EvaluationStage::kConstant) && + return (state.earliest_eval_stage <= sem::EvaluationStage::kConstant) && ty->IsAnyOf<Any, type::AbstractNumeric>(); } @@ -365,7 +365,7 @@ } bool match_ia(MatchState& state, const type::Type* ty) { - return (state.earliest_eval_stage == sem::EvaluationStage::kConstant) && + return (state.earliest_eval_stage <= sem::EvaluationStage::kConstant) && ty->IsAnyOf<Any, type::AbstractInt>(); }
diff --git a/src/tint/resolver/resolver.cc b/src/tint/resolver/resolver.cc index cec6e5a..09c4518 100644 --- a/src/tint/resolver/resolver.cc +++ b/src/tint/resolver/resolver.cc
@@ -2620,13 +2620,18 @@ } const constant::Value* val = nullptr; - if (auto r = const_eval_.Literal(ty, literal)) { - val = r.Get(); - } else { - return nullptr; + auto stage = sem::EvaluationStage::kConstant; + if (skip_const_eval_.Contains(literal)) { + stage = sem::EvaluationStage::kNotEvaluated; } - return builder_->create<sem::Expression>(literal, ty, sem::EvaluationStage::kConstant, - current_statement_, std::move(val), + if (stage == sem::EvaluationStage::kConstant) { + if (auto r = const_eval_.Literal(ty, literal)) { + val = r.Get(); + } else { + return nullptr; + } + } + return builder_->create<sem::Expression>(literal, ty, stage, current_statement_, std::move(val), /* has_side_effects */ false); } @@ -2899,29 +2904,36 @@ } const constant::Value* value = nullptr; - if (stage == sem::EvaluationStage::kConstant) { - if (op.const_eval_fn) { - if (skip_const_eval_.Contains(expr)) { - stage = sem::EvaluationStage::kNotEvaluated; - } else if (skip_const_eval_.Contains(expr->rhs)) { - // Only the rhs should be short-circuited, use the lhs value - value = lhs->ConstantValue(); + if (skip_const_eval_.Contains(expr)) { + // This expression is short-circuited by an ancestor expression. + // Do not const-eval. + stage = sem::EvaluationStage::kNotEvaluated; + } else if (lhs->Stage() == sem::EvaluationStage::kConstant && + rhs->Stage() == sem::EvaluationStage::kNotEvaluated) { + // Short-circuiting binary expression. Use the LHS value and stage. + value = lhs->ConstantValue(); + stage = sem::EvaluationStage::kConstant; + } else if (stage == sem::EvaluationStage::kConstant) { + // Both LHS and RHS have expressions that are constant evaluation stage. + if (op.const_eval_fn) { // Do we have a @const operator? + // Yes. Perform any required abstract argument values implicit conversions to the + // overload parameter types, and const-eval. + utils::Vector const_args{lhs->ConstantValue(), rhs->ConstantValue()}; + // Implicit conversion (e.g. AInt -> AFloat) + if (!Convert(const_args[0], op.lhs, lhs->Declaration()->source)) { + return nullptr; + } + if (!Convert(const_args[1], op.rhs, rhs->Declaration()->source)) { + return nullptr; + } + if (auto r = (const_eval_.*op.const_eval_fn)(op.result, const_args, expr->source)) { + value = r.Get(); } else { - auto const_args = utils::Vector{lhs->ConstantValue(), rhs->ConstantValue()}; - // Implicit conversion (e.g. AInt -> AFloat) - if (!Convert(const_args[0], op.lhs, lhs->Declaration()->source)) { - return nullptr; - } - if (!Convert(const_args[1], op.rhs, rhs->Declaration()->source)) { - return nullptr; - } - if (auto r = (const_eval_.*op.const_eval_fn)(op.result, const_args, expr->source)) { - value = r.Get(); - } else { - return nullptr; - } + return nullptr; } } else { + // The arguments have constant values, but the operator cannot be const-evaluated. This + // can only be evaluated at runtime. stage = sem::EvaluationStage::kRuntime; } }
diff --git a/src/tint/sem/expression.cc b/src/tint/sem/expression.cc index cf338c4..ebea34c 100644 --- a/src/tint/sem/expression.cc +++ b/src/tint/sem/expression.cc
@@ -39,6 +39,9 @@ has_side_effects_(has_side_effects) { TINT_ASSERT(Semantic, type_); TINT_ASSERT(Semantic, (constant != nullptr) == (stage == EvaluationStage::kConstant)); + if (constant != nullptr) { + TINT_ASSERT(Semantic, type_ == constant->Type()); + } } Expression::~Expression() = default;
diff --git a/test/tint/bug/chromium/1403752.wgsl b/test/tint/bug/chromium/1403752.wgsl new file mode 100644 index 0000000..9ab1131 --- /dev/null +++ b/test/tint/bug/chromium/1403752.wgsl
@@ -0,0 +1 @@ +fn d(){var j:i32;for(;0<0&&j<0-8;){}}
diff --git a/test/tint/bug/chromium/1403752.wgsl.expected.dxc.hlsl b/test/tint/bug/chromium/1403752.wgsl.expected.dxc.hlsl new file mode 100644 index 0000000..c921ec3 --- /dev/null +++ b/test/tint/bug/chromium/1403752.wgsl.expected.dxc.hlsl
@@ -0,0 +1,12 @@ +[numthreads(1, 1, 1)] +void unused_entry_point() { + return; +} + +void d() { + int j = 0; + { + for(; false; ) { + } + } +}
diff --git a/test/tint/bug/chromium/1403752.wgsl.expected.fxc.hlsl b/test/tint/bug/chromium/1403752.wgsl.expected.fxc.hlsl new file mode 100644 index 0000000..c921ec3 --- /dev/null +++ b/test/tint/bug/chromium/1403752.wgsl.expected.fxc.hlsl
@@ -0,0 +1,12 @@ +[numthreads(1, 1, 1)] +void unused_entry_point() { + return; +} + +void d() { + int j = 0; + { + for(; false; ) { + } + } +}
diff --git a/test/tint/bug/chromium/1403752.wgsl.expected.glsl b/test/tint/bug/chromium/1403752.wgsl.expected.glsl new file mode 100644 index 0000000..e670d5c --- /dev/null +++ b/test/tint/bug/chromium/1403752.wgsl.expected.glsl
@@ -0,0 +1,14 @@ +#version 310 es + +layout(local_size_x = 1, local_size_y = 1, local_size_z = 1) in; +void unused_entry_point() { + return; +} +void d() { + int j = 0; + { + for(; false; ) { + } + } +} +
diff --git a/test/tint/bug/chromium/1403752.wgsl.expected.msl b/test/tint/bug/chromium/1403752.wgsl.expected.msl new file mode 100644 index 0000000..d427a04 --- /dev/null +++ b/test/tint/bug/chromium/1403752.wgsl.expected.msl
@@ -0,0 +1,9 @@ +#include <metal_stdlib> + +using namespace metal; +void d() { + int j = 0; + for(; false; ) { + } +} +
diff --git a/test/tint/bug/chromium/1403752.wgsl.expected.spvasm b/test/tint/bug/chromium/1403752.wgsl.expected.spvasm new file mode 100644 index 0000000..a7165e4 --- /dev/null +++ b/test/tint/bug/chromium/1403752.wgsl.expected.spvasm
@@ -0,0 +1,42 @@ +; SPIR-V +; Version: 1.3 +; Generator: Google Tint Compiler; 0 +; Bound: 19 +; 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 %d "d" + OpName %j "j" + %void = OpTypeVoid + %1 = OpTypeFunction %void + %int = OpTypeInt 32 1 +%_ptr_Function_int = OpTypePointer Function %int + %10 = OpConstantNull %int + %bool = OpTypeBool + %true = OpConstantTrue %bool +%unused_entry_point = OpFunction %void None %1 + %4 = OpLabel + OpReturn + OpFunctionEnd + %d = OpFunction %void None %1 + %6 = OpLabel + %j = OpVariable %_ptr_Function_int Function %10 + OpBranch %11 + %11 = OpLabel + OpLoopMerge %12 %13 None + OpBranch %14 + %14 = OpLabel + OpSelectionMerge %17 None + OpBranchConditional %true %18 %17 + %18 = OpLabel + OpBranch %12 + %17 = OpLabel + OpBranch %13 + %13 = OpLabel + OpBranch %11 + %12 = OpLabel + OpReturn + OpFunctionEnd
diff --git a/test/tint/bug/chromium/1403752.wgsl.expected.wgsl b/test/tint/bug/chromium/1403752.wgsl.expected.wgsl new file mode 100644 index 0000000..7cabe2e --- /dev/null +++ b/test/tint/bug/chromium/1403752.wgsl.expected.wgsl
@@ -0,0 +1,5 @@ +fn d() { + var j : i32; + for(; ((0 < 0) && (j < (0 - 8))); ) { + } +}