From 70eb1d3573bf9f3e6b10cf82e6d339fc2563ca48 Mon Sep 17 00:00:00 2001 From: David Neto Date: Sat, 24 Jul 2021 06:11:52 +0000 Subject: [PATCH] spirv-reader: polyfill scalar faceForward Bug: tint:1018 Change-Id: I912c6deaed4e3c3f4c5dfb76e7ed7e917b4c6498 Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/58820 Kokoro: Kokoro Commit-Queue: David Neto Auto-Submit: David Neto Reviewed-by: Ben Clayton --- src/reader/spirv/function.cc | 121 ++++++++++++------ .../spirv/function_glsl_std_450_test.cc | 95 +++++++++++++- 2 files changed, 173 insertions(+), 43 deletions(-) diff --git a/src/reader/spirv/function.cc b/src/reader/spirv/function.cc index 819b9a30a3..43401f580f 100644 --- a/src/reader/spirv/function.cc +++ b/src/reader/spirv/function.cc @@ -3988,43 +3988,65 @@ TypedExpression FunctionEmitter::EmitGlslStd450ExtInst( auto* result_type = parser_impl_.ConvertType(inst.type_id()); - if ((ext_opcode == GLSLstd450Normalize) && result_type->IsScalar()) { - // WGSL does not have scalar form of the normalize builtin. - // The answer would be 1 anyway, so return that directly. - return {ty_.F32(), - create( - Source{}, create(Source{}, 1.0f))}; - } - if ((ext_opcode == GLSLstd450Refract) && result_type->IsScalar()) { - // WGSL does not have scalar form of the refract builtin. - // It's a complicated expression. Implement it by /computing it in two - // dimensions, but with a 0-valued y component in both the incident and - // normal vectors, then take the x component of that result. - auto incident = MakeOperand(inst, 2); - auto normal = MakeOperand(inst, 3); - auto eta = MakeOperand(inst, 4); - TINT_ASSERT(Reader, incident.type->Is()); - TINT_ASSERT(Reader, normal.type->Is()); - TINT_ASSERT(Reader, eta.type->Is()); - if (!success()) { - return {}; + if (result_type->IsScalar()) { + // Some GLSLstd450 builtins have scalar forms not supported by WGSL. + // Emulate them. + switch (ext_opcode) { + case GLSLstd450Normalize: + // WGSL does not have scalar form of the normalize builtin. + // The answer would be 1 anyway, so return that directly. + return {ty_.F32(), builder_.Expr(1.0f)}; + case GLSLstd450FaceForward: { + // If dot(Nref, Incident) < 0, the result is Normal, otherwise -Normal. + // Also: select(-normal,normal, Incident*Nref < 0) + // (The dot product of scalars is their product.) + // Use a multiply instead of comparing floating point signs. It should + // be among the fastest operations on a GPU. + auto normal = MakeOperand(inst, 2); + auto incident = MakeOperand(inst, 3); + auto nref = MakeOperand(inst, 4); + TINT_ASSERT(Reader, normal.type->Is()); + TINT_ASSERT(Reader, incident.type->Is()); + TINT_ASSERT(Reader, nref.type->Is()); + return {ty_.F32(), + builder_.Call( + Source{}, "select", + ast::ExpressionList{ + create( + Source{}, ast::UnaryOp::kNegation, normal.expr), + normal.expr, + create( + Source{}, ast::BinaryOp::kLessThan, + builder_.Mul({}, incident.expr, nref.expr), + builder_.Expr(0.0f))})}; + } + + case GLSLstd450Refract: { + // It's a complicated expression. Compute it in two dimensions, but + // with a 0-valued y component in both the incident and normal vectors, + // then take the x component of that result. + auto incident = MakeOperand(inst, 2); + auto normal = MakeOperand(inst, 3); + auto eta = MakeOperand(inst, 4); + TINT_ASSERT(Reader, incident.type->Is()); + TINT_ASSERT(Reader, normal.type->Is()); + TINT_ASSERT(Reader, eta.type->Is()); + if (!success()) { + return {}; + } + const Type* f32 = eta.type; + return {f32, + builder_.MemberAccessor( + builder_.Call( + Source{}, "refract", + ast::ExpressionList{ + builder_.vec2(incident.expr, 0.0f), + builder_.vec2(normal.expr, 0.0f), eta.expr}), + "x")}; + } + default: + break; } - const Type* f32 = eta.type; - const Type* vec2 = ty_.Vector(f32, 2); - return { - f32, - builder_.MemberAccessor( - builder_.Call( - Source{}, "refract", - ast::ExpressionList{ - builder_.Construct(vec2->Build(builder_), - ast::ExpressionList{ - incident.expr, builder_.Expr(0.0f)}), - builder_.Construct( - vec2->Build(builder_), - ast::ExpressionList{normal.expr, builder_.Expr(0.0f)}), - eta.expr}), - "x")}; } const auto name = GetGlslStd450FuncName(ext_opcode); @@ -4654,18 +4676,33 @@ const Type* FunctionEmitter::RemapStorageClass(const Type* type, void FunctionEmitter::FindValuesNeedingNamedOrHoistedDefinition() { // Mark vector operands of OpVectorShuffle as needing a named definition, // but only if they are defined in this function as well. + auto require_named_const_def = [&](const spvtools::opt::Instruction& inst, + int in_operand_index) { + const auto id = inst.GetSingleWordInOperand(in_operand_index); + auto* const operand_def = GetDefInfo(id); + if (operand_def) { + operand_def->requires_named_const_def = true; + } + }; for (auto& id_def_info_pair : def_info_) { const auto& inst = id_def_info_pair.second->inst; const auto opcode = inst.opcode(); if ((opcode == SpvOpVectorShuffle) || (opcode == SpvOpOuterProduct)) { // We might access the vector operands multiple times. Make sure they // are evaluated only once. - for (auto vector_arg : std::array{0, 1}) { - auto id = inst.GetSingleWordInOperand(vector_arg); - auto* operand_def = GetDefInfo(id); - if (operand_def) { - operand_def->requires_named_const_def = true; - } + require_named_const_def(inst, 0); + require_named_const_def(inst, 1); + } + if (parser_impl_.IsGlslExtendedInstruction(inst)) { + // Some emulations of GLSLstd450 instructions evaluate certain operands + // multiple times. Ensure their expressions are evaluated only once. + switch (inst.GetSingleWordInOperand(1)) { + case GLSLstd450FaceForward: + // The "normal" operand expression is used twice in code generation. + require_named_const_def(inst, 2); + break; + default: + break; } } } diff --git a/src/reader/spirv/function_glsl_std_450_test.cc b/src/reader/spirv/function_glsl_std_450_test.cc index 599a016bfd..0699005e1b 100644 --- a/src/reader/spirv/function_glsl_std_450_test.cc +++ b/src/reader/spirv/function_glsl_std_450_test.cc @@ -739,7 +739,6 @@ INSTANTIATE_TEST_SUITE_P( ::testing::ValuesIn(std::vector{ {"NClamp", "clamp"}, {"FClamp", "clamp"}, // WGSL FClamp promises more for NaN - {"FaceForward", "faceForward"}, {"Fma", "fma"}, {"FMix", "mix"}, {"SmoothStep", "smoothStep"}})); @@ -1820,6 +1819,100 @@ TEST_F(SpvParserTest, GlslStd450_Refract_Vector) { EXPECT_THAT(body, HasSubstr(expected)); } +TEST_F(SpvParserTest, GlslStd450_FaceForward_Scalar) { + const auto assembly = Preamble() + R"( + %99 = OpFAdd %float %f1 %f1 ; normal operand has only one use + %1 = OpExtInst %float %glsl FaceForward %99 %f2 %f3 + OpReturn + OpFunctionEnd + )"; + auto p = parser(test::Assemble(assembly)); + ASSERT_TRUE(p->BuildAndParseInternalModuleExceptFunctions()); + auto fe = p->function_emitter(100); + EXPECT_TRUE(fe.EmitBody()) << p->error(); + const auto body = ToString(p->builder(), fe.ast_body()); + // The %99 sum only has one use. Ensure it is evaluated only once by + // making a let-declaration for it, since it is the normal operand to + // the builtin function, and code generation uses it twice. + const auto* expected = R"(VariableDeclStatement{ + VariableConst{ + x_99 + none + undefined + __f32 + { + Binary[not set]{ + Identifier[not set]{f1} + add + Identifier[not set]{f1} + } + } + } +} +VariableDeclStatement{ + VariableConst{ + x_1 + none + undefined + __f32 + { + Call[not set]{ + Identifier[not set]{select} + ( + UnaryOp[not set]{ + negation + Identifier[not set]{x_99} + } + Identifier[not set]{x_99} + Binary[not set]{ + Binary[not set]{ + Identifier[not set]{f2} + multiply + Identifier[not set]{f3} + } + less_than + ScalarConstructor[not set]{0.000000} + } + ) + } + } + } +})"; + + EXPECT_THAT(body, HasSubstr(expected)) << body; +} + +TEST_F(SpvParserTest, GlslStd450_FaceForward_Vector) { + const auto assembly = Preamble() + R"( + %99 = OpFAdd %v2float %v2f1 %v2f1 + %1 = OpExtInst %v2float %glsl FaceForward %v2f1 %v2f2 %v2f3 + OpReturn + OpFunctionEnd + )"; + auto p = parser(test::Assemble(assembly)); + ASSERT_TRUE(p->BuildAndParseInternalModuleExceptFunctions()); + auto fe = p->function_emitter(100); + EXPECT_TRUE(fe.EmitBody()) << p->error(); + const auto body = ToString(p->builder(), fe.ast_body()); + const auto* expected = R"(VariableConst{ + x_1 + none + undefined + __vec_2__f32 + { + Call[not set]{ + Identifier[not set]{faceForward} + ( + Identifier[not set]{v2f1} + Identifier[not set]{v2f2} + Identifier[not set]{v2f3} + ) + } + })"; + + EXPECT_THAT(body, HasSubstr(expected)); +} + } // namespace } // namespace spirv } // namespace reader