spirv-writer: Fix phi for short-circuiting operators
The Phi in the merge block was taking the value of the RHS
from the wrong basic block ID. Instead of taking it from
the first block of the expression for the RHS, take it from
the last block of the expression for the RHS.
Bug: tint:355
Change-Id: I1b79a1b107459fd420e39963ad7ab2e89bc4494f
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/33640
Commit-Queue: David Neto <dneto@google.com>
Auto-Submit: David Neto <dneto@google.com>
Reviewed-by: dan sinclair <dsinclair@chromium.org>
diff --git a/src/writer/spirv/builder.cc b/src/writer/spirv/builder.cc
index f9dd6b4..0d18d84 100644
--- a/src/writer/spirv/builder.cc
+++ b/src/writer/spirv/builder.cc
@@ -1481,6 +1481,8 @@
}
lhs_id = GenerateLoadIfNeeded(expr->lhs()->result_type(), lhs_id);
+ // Get the ID of the basic block where control flow will diverge. It's the
+ // last basic block generated for the left-hand-side of the operator.
auto original_label_id = current_label_id_;
auto type_id = GenerateTypeIfNeeded(expr->result_type());
@@ -1517,6 +1519,9 @@
}
rhs_id = GenerateLoadIfNeeded(expr->rhs()->result_type(), rhs_id);
+ // Get the block ID of the last basic block generated for the right-hand-side
+ // expression. That block will be an immediate predecessor to the merge block.
+ auto rhs_block_id = current_label_id_;
push_function_inst(spv::Op::OpBranch, {Operand::Int(merge_block_id)});
// Output the merge block
@@ -1528,7 +1533,7 @@
push_function_inst(spv::Op::OpPhi,
{Operand::Int(type_id), result, Operand::Int(lhs_id),
Operand::Int(original_label_id), Operand::Int(rhs_id),
- Operand::Int(block_id)});
+ Operand::Int(rhs_block_id)});
return result_id;
}
diff --git a/src/writer/spirv/builder_binary_expression_test.cc b/src/writer/spirv/builder_binary_expression_test.cc
index 1b9c555..3a761c9 100644
--- a/src/writer/spirv/builder_binary_expression_test.cc
+++ b/src/writer/spirv/builder_binary_expression_test.cc
@@ -903,6 +903,98 @@
)");
}
+TEST_F(BuilderTest, Binary_logicalOr_Nested_LogicalAnd) {
+ ast::type::BoolType bool_ty;
+
+ // Test an expression like
+ // a || (b && c)
+ // From: crbug.com/tint/355
+
+ auto* logical_and_expr = create<ast::BinaryExpression>(
+ ast::BinaryOp::kLogicalAnd,
+ create<ast::ScalarConstructorExpression>(
+ create<ast::BoolLiteral>(&bool_ty, true)),
+ create<ast::ScalarConstructorExpression>(
+ create<ast::BoolLiteral>(&bool_ty, false)));
+
+ ast::BinaryExpression expr(ast::BinaryOp::kLogicalOr,
+ create<ast::ScalarConstructorExpression>(
+ create<ast::BoolLiteral>(&bool_ty, true)),
+ logical_and_expr);
+
+ ASSERT_TRUE(td.DetermineResultType(&expr)) << td.error();
+
+ b.push_function(Function{});
+ b.GenerateLabel(b.next_id());
+
+ EXPECT_EQ(b.GenerateBinaryExpression(&expr), 10u) << b.error();
+ EXPECT_EQ(DumpInstructions(b.types()), R"(%2 = OpTypeBool
+%3 = OpConstantTrue %2
+%8 = OpConstantFalse %2
+)");
+ EXPECT_EQ(DumpInstructions(b.functions()[0].instructions()),
+ R"(%1 = OpLabel
+OpSelectionMerge %4 None
+OpBranchConditional %3 %4 %5
+%5 = OpLabel
+OpSelectionMerge %6 None
+OpBranchConditional %3 %7 %6
+%7 = OpLabel
+OpBranch %6
+%6 = OpLabel
+%9 = OpPhi %2 %3 %5 %8 %7
+OpBranch %4
+%4 = OpLabel
+%10 = OpPhi %2 %3 %1 %9 %6
+)");
+}
+
+TEST_F(BuilderTest, Binary_logicalAnd_Nested_LogicalOr) {
+ ast::type::BoolType bool_ty;
+
+ // Test an expression like
+ // a && (b || c)
+ // From: crbug.com/tint/355
+
+ auto* logical_or_expr = create<ast::BinaryExpression>(
+ ast::BinaryOp::kLogicalOr,
+ create<ast::ScalarConstructorExpression>(
+ create<ast::BoolLiteral>(&bool_ty, true)),
+ create<ast::ScalarConstructorExpression>(
+ create<ast::BoolLiteral>(&bool_ty, false)));
+
+ ast::BinaryExpression expr(ast::BinaryOp::kLogicalAnd,
+ create<ast::ScalarConstructorExpression>(
+ create<ast::BoolLiteral>(&bool_ty, true)),
+ logical_or_expr);
+
+ ASSERT_TRUE(td.DetermineResultType(&expr)) << td.error();
+
+ b.push_function(Function{});
+ b.GenerateLabel(b.next_id());
+
+ EXPECT_EQ(b.GenerateBinaryExpression(&expr), 10u) << b.error();
+ EXPECT_EQ(DumpInstructions(b.types()), R"(%2 = OpTypeBool
+%3 = OpConstantTrue %2
+%8 = OpConstantFalse %2
+)");
+ EXPECT_EQ(DumpInstructions(b.functions()[0].instructions()),
+ R"(%1 = OpLabel
+OpSelectionMerge %4 None
+OpBranchConditional %3 %5 %4
+%5 = OpLabel
+OpSelectionMerge %6 None
+OpBranchConditional %3 %6 %7
+%7 = OpLabel
+OpBranch %6
+%6 = OpLabel
+%9 = OpPhi %2 %3 %5 %8 %7
+OpBranch %4
+%4 = OpLabel
+%10 = OpPhi %2 %3 %1 %9 %6
+)");
+}
+
TEST_F(BuilderTest, Binary_LogicalOr) {
ast::type::I32Type i32;
