From 1c200cffd051606f03f8859859fc8e6baa6bee08 Mon Sep 17 00:00:00 2001 From: dan sinclair Date: Tue, 12 Jan 2021 22:00:58 +0000 Subject: [PATCH] Remove outerProduct. The community decided to remove outerProduct from WGSL. This Cl removes the pieces from Tint. Change-Id: Ib1735867e4a7ca852a72549fc8c9bd86e8de22b0 Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/37600 Commit-Queue: dan sinclair Commit-Queue: Ben Clayton Auto-Submit: dan sinclair Reviewed-by: Ben Clayton --- docs/translations.md | 1 - src/ast/intrinsic.cc | 3 - src/ast/intrinsic.h | 1 - src/reader/spirv/function.cc | 2 - src/reader/spirv/function_arithmetic_test.cc | 32 ------ src/type_determiner.cc | 24 ---- src/type_determiner_test.cc | 50 --------- src/writer/hlsl/generator_impl.cc | 53 --------- .../hlsl/generator_impl_intrinsic_test.cc | 20 ---- src/writer/msl/generator_impl.cc | 105 +++++------------- .../msl/generator_impl_intrinsic_test.cc | 19 ---- src/writer/spirv/builder.cc | 2 - src/writer/spirv/builder_intrinsic_test.cc | 32 ------ 13 files changed, 25 insertions(+), 319 deletions(-) diff --git a/docs/translations.md b/docs/translations.md index d6ff55e2fa..5688952355 100644 --- a/docs/translations.md +++ b/docs/translations.md @@ -88,7 +88,6 @@ decorated with `NonWritable` or each member of the struct can be decorated with | mix | GLSLstd450FMix | mix | mix | | modf | GLSLstd450Modf | | | | normalize | GLSLstd450Normalize | normalize | normalize | -| outerProduct | SpvOpOuterProduct | | | | pow | GLSLstd450Pow | pow | pow | | reflect | GLSLstd450Reflect | reflect | reflect | | reverseBits | SpvOpBitReverse | reverse_bits | reversebits | diff --git a/src/ast/intrinsic.cc b/src/ast/intrinsic.cc index c4ce9c14c2..73f3ab3dcb 100644 --- a/src/ast/intrinsic.cc +++ b/src/ast/intrinsic.cc @@ -162,9 +162,6 @@ std::ostream& operator<<(std::ostream& out, Intrinsic i) { case Intrinsic::kNormalize: out << "normalize"; break; - case Intrinsic::kOuterProduct: - out << "outerProduct"; - break; case Intrinsic::kPow: out << "pow"; break; diff --git a/src/ast/intrinsic.h b/src/ast/intrinsic.h index 568c9ee10d..9de55b1661 100644 --- a/src/ast/intrinsic.h +++ b/src/ast/intrinsic.h @@ -70,7 +70,6 @@ enum class Intrinsic { kMix, kModf, kNormalize, - kOuterProduct, kPow, kReflect, kReverseBits, diff --git a/src/reader/spirv/function.cc b/src/reader/spirv/function.cc index fcfcc33163..93e7cd83cf 100644 --- a/src/reader/spirv/function.cc +++ b/src/reader/spirv/function.cc @@ -472,8 +472,6 @@ ast::Intrinsic GetIntrinsic(SpvOp opcode) { return ast::Intrinsic::kReverseBits; case SpvOpDot: return ast::Intrinsic::kDot; - case SpvOpOuterProduct: - return ast::Intrinsic::kOuterProduct; default: break; } diff --git a/src/reader/spirv/function_arithmetic_test.cc b/src/reader/spirv/function_arithmetic_test.cc index b474a27409..044fca5090 100644 --- a/src/reader/spirv/function_arithmetic_test.cc +++ b/src/reader/spirv/function_arithmetic_test.cc @@ -1099,38 +1099,6 @@ TEST_F(SpvBinaryArithTestBasic, Dot) { << ToString(p->get_module(), fe.ast_body()); } -TEST_F(SpvBinaryArithTestBasic, OuterProduct) { - const auto assembly = CommonTypes() + R"( - %100 = OpFunction %void None %voidfn - %entry = OpLabel - %1 = OpCopyObject %v2float %v2float_50_60 - %2 = OpCopyObject %v2float %v2float_60_50 - %3 = OpOuterProduct %m2v2float %1 %2 - OpReturn - OpFunctionEnd -)"; - auto p = parser(test::Assemble(assembly)); - ASSERT_TRUE(p->BuildAndParseInternalModuleExceptFunctions()) << assembly; - FunctionEmitter fe(p.get(), *spirv_function(p.get(), 100)); - EXPECT_TRUE(fe.EmitBody()) << p->error(); - EXPECT_THAT(ToString(p->get_module(), fe.ast_body()), - HasSubstr(R"(VariableConst{ - x_3 - none - __mat_2_2__f32 - { - Call[not set]{ - Identifier[not set]{outerProduct} - ( - Identifier[not set]{x_1} - Identifier[not set]{x_2} - ) - } - } - })")) - << ToString(p->get_module(), fe.ast_body()); -} - // TODO(dneto): OpSRem. Missing from WGSL // https://github.com/gpuweb/gpuweb/issues/702 diff --git a/src/type_determiner.cc b/src/type_determiner.cc index eaa95bf02f..b8aab37737 100644 --- a/src/type_determiner.cc +++ b/src/type_determiner.cc @@ -721,28 +721,6 @@ bool TypeDeterminer::DetermineIntrinsic(ast::IdentifierExpression* ident, expr->func()->set_result_type(mod_->create()); return true; } - if (ident->intrinsic() == ast::Intrinsic::kOuterProduct) { - if (expr->params().size() != 2) { - set_error(expr->source(), "incorrect number of parameters for " + - mod_->SymbolToName(ident->symbol())); - return false; - } - - auto* param0_type = expr->params()[0]->result_type()->UnwrapPtrIfNeeded(); - auto* param1_type = expr->params()[1]->result_type()->UnwrapPtrIfNeeded(); - if (!param0_type->Is() || - !param1_type->Is()) { - set_error(expr->source(), "invalid parameter type for " + - mod_->SymbolToName(ident->symbol())); - return false; - } - - expr->func()->set_result_type(mod_->create( - mod_->create(), - param0_type->As()->size(), - param1_type->As()->size())); - return true; - } if (ident->intrinsic() == ast::Intrinsic::kSelect) { if (expr->params().size() != 3) { set_error(expr->source(), "incorrect number of parameters for " + @@ -1017,8 +995,6 @@ bool TypeDeterminer::SetIntrinsicIfNeeded(ast::IdentifierExpression* ident) { ident->set_intrinsic(ast::Intrinsic::kModf); } else if (name == "normalize") { ident->set_intrinsic(ast::Intrinsic::kNormalize); - } else if (name == "outerProduct") { - ident->set_intrinsic(ast::Intrinsic::kOuterProduct); } else if (name == "pow") { ident->set_intrinsic(ast::Intrinsic::kPow); } else if (name == "reflect") { diff --git a/src/type_determiner_test.cc b/src/type_determiner_test.cc index 46545c412a..87f6ed0adc 100644 --- a/src/type_determiner_test.cc +++ b/src/type_determiner_test.cc @@ -1617,55 +1617,6 @@ TEST_F(TypeDeterminerTest, Intrinsic_Select_TooManyParams) { "incorrect number of parameters for select expected 3 got 4"); } -TEST_F(TypeDeterminerTest, Intrinsic_OuterProduct) { - auto* var1 = Var( // source - "v3", ast::StorageClass::kNone, ty.vec3()); - auto* var2 = Var( // source - "v2", ast::StorageClass::kNone, ty.vec2()); - mod->AddGlobalVariable(var1); - mod->AddGlobalVariable(var2); - - auto* expr = Call("outerProduct", "v3", "v2"); - - // Register the variable - EXPECT_TRUE(td()->Determine()); - EXPECT_TRUE(td()->DetermineResultType(expr)); - - ASSERT_NE(expr->result_type(), nullptr); - ASSERT_TRUE(expr->result_type()->Is()); - - auto* mat = expr->result_type()->As(); - EXPECT_TRUE(mat->type()->Is()); - EXPECT_EQ(mat->rows(), 3u); - EXPECT_EQ(mat->columns(), 2u); -} - -TEST_F(TypeDeterminerTest, Intrinsic_OuterProduct_TooFewParams) { - auto* var2 = Var( // source - "v2", ast::StorageClass::kNone, ty.vec2()); - mod->AddGlobalVariable(var2); - - auto* expr = Call("outerProduct", "v2"); - - // Register the variable - EXPECT_TRUE(td()->Determine()); - EXPECT_FALSE(td()->DetermineResultType(expr)); - EXPECT_EQ(td()->error(), "incorrect number of parameters for outerProduct"); -} - -TEST_F(TypeDeterminerTest, Intrinsic_OuterProduct_TooManyParams) { - auto* var2 = Var( // source - "v2", ast::StorageClass::kNone, ty.vec2()); - mod->AddGlobalVariable(var2); - - auto* expr = Call("outerProduct", "v2", "v2", "v2"); - - // Register the variable - EXPECT_TRUE(td()->Determine()); - EXPECT_FALSE(td()->DetermineResultType(expr)); - EXPECT_EQ(td()->error(), "incorrect number of parameters for outerProduct"); -} - using UnaryOpExpressionTest = TypeDeterminerTestWithParam; TEST_P(UnaryOpExpressionTest, Expr_UnaryOp) { auto op = GetParam(); @@ -1798,7 +1749,6 @@ INSTANTIATE_TEST_SUITE_P( IntrinsicData{"mix", ast::Intrinsic::kMix}, IntrinsicData{"modf", ast::Intrinsic::kModf}, IntrinsicData{"normalize", ast::Intrinsic::kNormalize}, - IntrinsicData{"outerProduct", ast::Intrinsic::kOuterProduct}, IntrinsicData{"pow", ast::Intrinsic::kPow}, IntrinsicData{"reflect", ast::Intrinsic::kReflect}, IntrinsicData{"reverseBits", ast::Intrinsic::kReverseBits}, diff --git a/src/writer/hlsl/generator_impl.cc b/src/writer/hlsl/generator_impl.cc index d95769fba4..4c6067b3d0 100644 --- a/src/writer/hlsl/generator_impl.cc +++ b/src/writer/hlsl/generator_impl.cc @@ -595,59 +595,6 @@ bool GeneratorImpl::EmitCall(std::ostream& pre, } else if (ident->intrinsic() == ast::Intrinsic::kIsNormal) { error_ = "is_normal not supported in HLSL backend yet"; return false; - } else if (ident->intrinsic() == ast::Intrinsic::kOuterProduct) { - error_ = "outer_product not supported yet"; - return false; - // TODO(dsinclair): This gets tricky. We need to generate two variables to - // hold the outer_product expressions, but we maybe inside an expression - // ourselves. So, this will need to, possibly, output the variables - // _before_ the expression which contains the outer product. - // - // This then has the follow on, what if we have `(false && - // outer_product())` in that case, we shouldn't evaluate the expressions - // at all because of short circuting. - // - // So .... this turns out to be hard ... - - // // We create variables to hold the two parameters in case they're - // // function calls with side effects. - // auto* param0 = param[0].get(); - // auto* name0 = generate_name("outer_product_expr_0"); - - // auto* param1 = param[1].get(); - // auto* name1 = generate_name("outer_product_expr_1"); - - // make_indent(out); - // if (!EmitType(out, expr->result_type(), "")) { - // return false; - // } - // out << "("; - - // auto param1_type = params[1]->result_type()->UnwrapPtrIfNeeded(); - // if (!param1_type->Is()) { - // error_ = "invalid param type in outer_product got: " + - // param1_type->type_name(); - // return false; - // } - - // for (uint32_t i = 0; i < - // param1_type->As()->size(); ++i) { - // if (i > 0) { - // out << ", "; - // } - - // if (!EmitExpression(pre, out, params[0].get())) { - // return false; - // } - // out << " * "; - - // if (!EmitExpression(pre, out, params[1].get())) { - // return false; - // } - // out << "[" << i << "]"; - // } - - // out << ")"; } else { auto name = generate_intrinsic_name(ident->intrinsic()); if (name.empty()) { diff --git a/src/writer/hlsl/generator_impl_intrinsic_test.cc b/src/writer/hlsl/generator_impl_intrinsic_test.cc index 1841ef6346..2658f740b7 100644 --- a/src/writer/hlsl/generator_impl_intrinsic_test.cc +++ b/src/writer/hlsl/generator_impl_intrinsic_test.cc @@ -70,26 +70,6 @@ TEST_F(HlslGeneratorImplTest_Intrinsic, DISABLED_Intrinsic_Select) { FAIL(); } -TEST_F(HlslGeneratorImplTest_Intrinsic, DISABLED_Intrinsic_OuterProduct) { - auto* a = Var("a", ast::StorageClass::kNone, ty.vec2()); - auto* b = Var("b", ast::StorageClass::kNone, ty.vec3()); - - auto* call = Call("outer_product", "a", "b"); - - td.RegisterVariableForTesting(a); - td.RegisterVariableForTesting(b); - - mod->AddGlobalVariable(a); - mod->AddGlobalVariable(b); - - ASSERT_TRUE(td.Determine()) << td.error(); - ASSERT_TRUE(td.DetermineResultType(call)) << td.error(); - - gen.increment_indent(); - ASSERT_TRUE(gen.EmitExpression(pre, out, call)) << gen.error(); - EXPECT_EQ(result(), " float3x2(a * b[0], a * b[1], a * b[2])"); -} - TEST_F(HlslGeneratorImplTest_Intrinsic, Intrinsic_Bad_Name) { EXPECT_EQ(gen.generate_intrinsic_name(ast::Intrinsic::kNone), ""); } diff --git a/src/writer/msl/generator_impl.cc b/src/writer/msl/generator_impl.cc index 5343a11ffe..a9509a88e8 100644 --- a/src/writer/msl/generator_impl.cc +++ b/src/writer/msl/generator_impl.cc @@ -487,89 +487,34 @@ bool GeneratorImpl::EmitCall(ast::CallExpression* expr) { } if (ident->IsIntrinsic()) { - const auto& params = expr->params(); - if (ident->intrinsic() == ast::Intrinsic::kOuterProduct) { - error_ = "outer_product not supported yet"; - return false; - // TODO(dsinclair): This gets tricky. We need to generate two variables to - // hold the outer_product expressions, but we maybe inside an expression - // ourselves. So, this will need to, possibly, output the variables - // _before_ the expression which contains the outer product. - // - // This then has the follow on, what if we have `(false && - // outer_product())` in that case, we shouldn't evaluate the expressions - // at all because of short circuting. - // - // So .... this turns out to be hard ... - - // // We create variables to hold the two parameters in case they're - // // function calls with side effects. - // auto* param0 = param[0].get(); - // auto* name0 = generate_name("outer_product_expr_0"); - - // auto* param1 = param[1].get(); - // auto* name1 = generate_name("outer_product_expr_1"); - - // make_indent(); - // if (!EmitType(expr->result_type(), "")) { - // return false; - // } - // out_ << "("; - - // auto param1_type = params[1]->result_type()->UnwrapPtrIfNeeded(); - // if (!param1_type->Is()) { - // error_ = "invalid param type in outer_product got: " + - // param1_type->type_name(); - // return false; - // } - - // for (uint32_t i = 0; i < - // param1_type->As()->size(); ++i) { - // if (i > 0) { - // out_ << ", "; - // } - - // if (!EmitExpression(params[0].get())) { - // return false; - // } - // out_ << " * "; - - // if (!EmitExpression(params[1].get())) { - // return false; - // } - // out_ << "[" << i << "]"; - // } - - // out_ << ")"; - } else { - auto name = generate_intrinsic_name(ident->intrinsic()); + auto name = generate_intrinsic_name(ident->intrinsic()); + if (name.empty()) { + if (ast::intrinsic::IsTextureIntrinsic(ident->intrinsic())) { + return EmitTextureCall(expr); + } + name = generate_builtin_name(ident); if (name.empty()) { - if (ast::intrinsic::IsTextureIntrinsic(ident->intrinsic())) { - return EmitTextureCall(expr); - } - name = generate_builtin_name(ident); - if (name.empty()) { - return false; - } + return false; } - - make_indent(); - out_ << name << "("; - - bool first = true; - for (auto* param : params) { - if (!first) { - out_ << ", "; - } - first = false; - - if (!EmitExpression(param)) { - return false; - } - } - - out_ << ")"; } + + make_indent(); + out_ << name << "("; + + bool first = true; + const auto& params = expr->params(); + for (auto* param : params) { + if (!first) { + out_ << ", "; + } + first = false; + + if (!EmitExpression(param)) { + return false; + } + } + + out_ << ")"; return true; } diff --git a/src/writer/msl/generator_impl_intrinsic_test.cc b/src/writer/msl/generator_impl_intrinsic_test.cc index af661cbf69..b1f0e92b00 100644 --- a/src/writer/msl/generator_impl_intrinsic_test.cc +++ b/src/writer/msl/generator_impl_intrinsic_test.cc @@ -65,25 +65,6 @@ INSTANTIATE_TEST_SUITE_P( IntrinsicData{ast::Intrinsic::kReverseBits, "reverse_bits"}, IntrinsicData{ast::Intrinsic::kSelect, "select"})); -TEST_F(MslGeneratorImplTest, DISABLED_Intrinsic_OuterProduct) { - auto* a = Var("a", ast::StorageClass::kNone, ty.vec2()); - auto* b = Var("b", ast::StorageClass::kNone, ty.vec3()); - - auto* call = Call("outer_product", "a", "b"); - td.RegisterVariableForTesting(a); - td.RegisterVariableForTesting(b); - - mod->AddGlobalVariable(a); - mod->AddGlobalVariable(b); - - ASSERT_TRUE(td.Determine()) << td.error(); - ASSERT_TRUE(td.DetermineResultType(call)) << td.error(); - - gen.increment_indent(); - ASSERT_TRUE(gen.EmitExpression(call)) << gen.error(); - EXPECT_EQ(gen.result(), " float3x2(a * b[0], a * b[1], a * b[2])"); -} - TEST_F(MslGeneratorImplTest, Intrinsic_Bad_Name) { EXPECT_EQ(gen.generate_intrinsic_name(ast::Intrinsic::kNone), ""); } diff --git a/src/writer/spirv/builder.cc b/src/writer/spirv/builder.cc index 1bdd166814..598a0db941 100644 --- a/src/writer/spirv/builder.cc +++ b/src/writer/spirv/builder.cc @@ -1940,8 +1940,6 @@ uint32_t Builder::GenerateIntrinsic(ast::IdentifierExpression* ident, op = spv::Op::OpIsInf; } else if (intrinsic == ast::Intrinsic::kIsNan) { op = spv::Op::OpIsNan; - } else if (intrinsic == ast::Intrinsic::kOuterProduct) { - op = spv::Op::OpOuterProduct; } else if (intrinsic == ast::Intrinsic::kReverseBits) { op = spv::Op::OpBitReverse; } else if (intrinsic == ast::Intrinsic::kSelect) { diff --git a/src/writer/spirv/builder_intrinsic_test.cc b/src/writer/spirv/builder_intrinsic_test.cc index c2dde98401..654317b2a6 100644 --- a/src/writer/spirv/builder_intrinsic_test.cc +++ b/src/writer/spirv/builder_intrinsic_test.cc @@ -349,38 +349,6 @@ INSTANTIATE_TEST_SUITE_P( IntrinsicData{"fwidthFine", "OpFwidthFine"}, IntrinsicData{"fwidthCoarse", "OpFwidthCoarse"})); -TEST_F(IntrinsicBuilderTest, Call_OuterProduct) { - auto* v2 = Var("v2", ast::StorageClass::kPrivate, ty.vec2()); - auto* v3 = Var("v3", ast::StorageClass::kPrivate, ty.vec3()); - - auto* expr = Call("outerProduct", "v2", "v3"); - - ASSERT_TRUE(td.DetermineResultType(expr)) << td.error(); - - b.push_function(Function{}); - ASSERT_TRUE(b.GenerateGlobalVariable(v2)) << b.error(); - ASSERT_TRUE(b.GenerateGlobalVariable(v3)) << b.error(); - - EXPECT_EQ(b.GenerateCallExpression(expr), 10u) << b.error(); - - EXPECT_EQ(DumpInstructions(b.types()), R"(%4 = OpTypeFloat 32 -%3 = OpTypeVector %4 2 -%2 = OpTypePointer Private %3 -%5 = OpConstantNull %3 -%1 = OpVariable %2 Private %5 -%8 = OpTypeVector %4 3 -%7 = OpTypePointer Private %8 -%9 = OpConstantNull %8 -%6 = OpVariable %7 Private %9 -%11 = OpTypeMatrix %3 3 -)"); - EXPECT_EQ(DumpInstructions(b.functions()[0].instructions()), - R"(%12 = OpLoad %3 %1 -%13 = OpLoad %8 %6 -%10 = OpOuterProduct %11 %12 %13 -)"); -} - TEST_F(IntrinsicBuilderTest, Call_Select) { auto* v3 = Var("v3", ast::StorageClass::kPrivate, ty.vec3()); auto* bool_v3 = Var("bool_v3", ast::StorageClass::kPrivate, ty.vec3());