From 5f994247a27356e8e8c1487d3fbd925f088341bf Mon Sep 17 00:00:00 2001 From: David Neto Date: Fri, 16 Jul 2021 18:17:05 +0000 Subject: [PATCH] reader: fix ordering of select operands Updates: spirv-reader hlsl-writer spirv-writer Bug: tint:1001 Change-Id: I7359d35c3384e164d149a6af03cbdc959a2eea76 Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/58400 Kokoro: Kokoro Reviewed-by: Ben Clayton Commit-Queue: Ben Clayton Auto-Submit: David Neto --- src/reader/spirv/function.cc | 23 +++++++++---------- src/reader/spirv/function_logical_test.cc | 14 +++++------ src/writer/hlsl/generator_impl.cc | 4 ++-- .../hlsl/generator_impl_intrinsic_test.cc | 4 ++-- src/writer/spirv/builder.cc | 4 ++-- 5 files changed, 24 insertions(+), 25 deletions(-) diff --git a/src/reader/spirv/function.cc b/src/reader/spirv/function.cc index 0998926ae1..fe3d320847 100644 --- a/src/reader/spirv/function.cc +++ b/src/reader/spirv/function.cc @@ -4960,28 +4960,27 @@ TypedExpression FunctionEmitter::MakeIntrinsicCall( TypedExpression FunctionEmitter::MakeSimpleSelect( const spvtools::opt::Instruction& inst) { auto condition = MakeOperand(inst, 0); - auto operand1 = MakeOperand(inst, 1); - auto operand2 = MakeOperand(inst, 2); + auto true_value = MakeOperand(inst, 1); + auto false_value = MakeOperand(inst, 2); // SPIR-V validation requires: // - the condition to be bool or bool vector, so we don't check it here. - // - operand1, operand2, and result type to match. + // - true_value false_value, and result type to match. // - you can't select over pointers or pointer vectors, unless you also have // a VariablePointers* capability, which is not allowed in by WebGPU. - auto* op_ty = operand1.type; + auto* op_ty = true_value.type; if (op_ty->Is() || op_ty->IsFloatScalar() || op_ty->IsIntegerScalar() || op_ty->Is()) { ast::ExpressionList params; - params.push_back(operand1.expr); - params.push_back(operand2.expr); + params.push_back(false_value.expr); + params.push_back(true_value.expr); // The condition goes last. params.push_back(condition.expr); - return {operand1.type, - create( - Source{}, - create( - Source{}, builder_.Symbols().Register("select")), - std::move(params))}; + return {op_ty, create( + Source{}, + create( + Source{}, builder_.Symbols().Register("select")), + std::move(params))}; } return {}; } diff --git a/src/reader/spirv/function_logical_test.cc b/src/reader/spirv/function_logical_test.cc index 180e115d5b..2d21a1b4f5 100644 --- a/src/reader/spirv/function_logical_test.cc +++ b/src/reader/spirv/function_logical_test.cc @@ -1160,9 +1160,9 @@ TEST_F(SpvLogicalTest, Select_BoolCond_BoolParams) { Call[not set]{ Identifier[not set]{select} ( - ScalarConstructor[not set]{true} ScalarConstructor[not set]{false} ScalarConstructor[not set]{true} + ScalarConstructor[not set]{true} ) } } @@ -1193,8 +1193,8 @@ TEST_F(SpvLogicalTest, Select_BoolCond_IntScalarParams) { Call[not set]{ Identifier[not set]{select} ( - ScalarConstructor[not set]{10u} ScalarConstructor[not set]{20u} + ScalarConstructor[not set]{10u} ScalarConstructor[not set]{true} ) } @@ -1226,8 +1226,8 @@ TEST_F(SpvLogicalTest, Select_BoolCond_FloatScalarParams) { Call[not set]{ Identifier[not set]{select} ( - ScalarConstructor[not set]{50.000000} ScalarConstructor[not set]{60.000000} + ScalarConstructor[not set]{50.000000} ScalarConstructor[not set]{true} ) } @@ -1264,13 +1264,13 @@ TEST_F(SpvLogicalTest, Select_BoolCond_VectorParams) { ( TypeConstructor[not set]{ __vec_2__u32 - ScalarConstructor[not set]{10u} ScalarConstructor[not set]{20u} + ScalarConstructor[not set]{10u} } TypeConstructor[not set]{ __vec_2__u32 - ScalarConstructor[not set]{20u} ScalarConstructor[not set]{10u} + ScalarConstructor[not set]{20u} } ScalarConstructor[not set]{true} ) @@ -1311,13 +1311,13 @@ TEST_F(SpvLogicalTest, Select_VecBoolCond_VectorParams) { ( TypeConstructor[not set]{ __vec_2__u32 - ScalarConstructor[not set]{10u} ScalarConstructor[not set]{20u} + ScalarConstructor[not set]{10u} } TypeConstructor[not set]{ __vec_2__u32 - ScalarConstructor[not set]{20u} ScalarConstructor[not set]{10u} + ScalarConstructor[not set]{20u} } TypeConstructor[not set]{ __vec_2__bool diff --git a/src/writer/hlsl/generator_impl.cc b/src/writer/hlsl/generator_impl.cc index dbaed5afbf..0a04858b8c 100644 --- a/src/writer/hlsl/generator_impl.cc +++ b/src/writer/hlsl/generator_impl.cc @@ -1247,8 +1247,8 @@ bool GeneratorImpl::EmitWorkgroupAtomicCall(std::ostream& out, bool GeneratorImpl::EmitSelectCall(std::ostream& out, ast::CallExpression* expr) { - auto* expr_true = expr->params()[0]; - auto* expr_false = expr->params()[1]; + auto* expr_false = expr->params()[0]; + auto* expr_true = expr->params()[1]; auto* expr_cond = expr->params()[2]; ScopedParen paren(out); if (!EmitExpression(out, expr_cond)) { diff --git a/src/writer/hlsl/generator_impl_intrinsic_test.cc b/src/writer/hlsl/generator_impl_intrinsic_test.cc index 1d5407a9a1..7bc6289728 100644 --- a/src/writer/hlsl/generator_impl_intrinsic_test.cc +++ b/src/writer/hlsl/generator_impl_intrinsic_test.cc @@ -281,7 +281,7 @@ TEST_F(HlslGeneratorImplTest_Intrinsic, Select_Scalar) { gen.increment_indent(); std::stringstream out; ASSERT_TRUE(gen.EmitExpression(out, call)) << gen.error(); - EXPECT_EQ(out.str(), "(true ? 1.0f : 2.0f)"); + EXPECT_EQ(out.str(), "(true ? 2.0f : 1.0f)"); } TEST_F(HlslGeneratorImplTest_Intrinsic, Select_Vector) { @@ -293,7 +293,7 @@ TEST_F(HlslGeneratorImplTest_Intrinsic, Select_Vector) { gen.increment_indent(); std::stringstream out; ASSERT_TRUE(gen.EmitExpression(out, call)) << gen.error(); - EXPECT_EQ(out.str(), "(bool2(true, false) ? int2(1, 2) : int2(3, 4))"); + EXPECT_EQ(out.str(), "(bool2(true, false) ? int2(3, 4) : int2(1, 2))"); } TEST_F(HlslGeneratorImplTest_Intrinsic, Modf_Scalar) { diff --git a/src/writer/spirv/builder.cc b/src/writer/spirv/builder.cc index 9382362123..b647fa3144 100644 --- a/src/writer/spirv/builder.cc +++ b/src/writer/spirv/builder.cc @@ -2448,8 +2448,8 @@ uint32_t Builder::GenerateIntrinsic(ast::CallExpression* call, case IntrinsicType::kSelect: { // Note: Argument order is different in WGSL and SPIR-V auto cond_id = get_param_as_value_id(2); - auto true_id = get_param_as_value_id(0); - auto false_id = get_param_as_value_id(1); + auto true_id = get_param_as_value_id(1); + auto false_id = get_param_as_value_id(0); if (!cond_id || !true_id || !false_id) { return 0; }