spirv-reader: polyfill scalar faceForward

Bug: tint:1018
Change-Id: I912c6deaed4e3c3f4c5dfb76e7ed7e917b4c6498
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/58820
Kokoro: Kokoro <noreply+kokoro@google.com>
Commit-Queue: David Neto <dneto@google.com>
Auto-Submit: David Neto <dneto@google.com>
Reviewed-by: Ben Clayton <bclayton@chromium.org>

src/reader/spirv/function.cc

@@ -3988,18 +3988,43 @@ TypedExpression FunctionEmitter::EmitGlslStd450ExtInst(
 
   auto* result_type = parser_impl_.ConvertType(inst.type_id());
 
-  if ((ext_opcode == GLSLstd450Normalize) && result_type->IsScalar()) {
+  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<F32>());
+        TINT_ASSERT(Reader, incident.type->Is<F32>());
+        TINT_ASSERT(Reader, nref.type->Is<F32>());
         return {ty_.F32(),
-            create<ast::ScalarConstructorExpression>(
+                builder_.Call(
-                Source{}, create<ast::FloatLiteral>(Source{}, 1.0f))};
+                    Source{}, "select",
+                    ast::ExpressionList{
+                        create<ast::UnaryOpExpression>(
+                            Source{}, ast::UnaryOp::kNegation, normal.expr),
+                        normal.expr,
+                        create<ast::BinaryExpression>(
+                            Source{}, ast::BinaryOp::kLessThan,
+                            builder_.Mul({}, incident.expr, nref.expr),
+                            builder_.Expr(0.0f))})};
       }
-  if ((ext_opcode == GLSLstd450Refract) && result_type->IsScalar()) {
+
-    // WGSL does not have scalar form of the refract builtin.
+      case GLSLstd450Refract: {
-    // It's a complicated expression.  Implement it by /computing it in two
+        // It's a complicated expression. Compute it in two dimensions, but
-    // dimensions, but with a 0-valued y component in both the incident and
+        // with a 0-valued y component in both the incident and normal vectors,
-    // normal vectors, then take the x component of that result.
+        // then take the x component of that result.
         auto incident = MakeOperand(inst, 2);
         auto normal = MakeOperand(inst, 3);
         auto eta = MakeOperand(inst, 4);
@@ -4010,22 +4035,19 @@ TypedExpression FunctionEmitter::EmitGlslStd450ExtInst(
           return {};
         }
         const Type* f32 = eta.type;
-    const Type* vec2 = ty_.Vector(f32, 2);
+        return {f32,
-    return {
-        f32,
                 builder_.MemberAccessor(
                     builder_.Call(
                         Source{}, "refract",
                         ast::ExpressionList{
-                    builder_.Construct(vec2->Build(builder_),
+                            builder_.vec2<float>(incident.expr, 0.0f),
-                                       ast::ExpressionList{
+                            builder_.vec2<float>(normal.expr, 0.0f), eta.expr}),
-                                           incident.expr, builder_.Expr(0.0f)}),
-                    builder_.Construct(
-                        vec2->Build(builder_),
-                        ast::ExpressionList{normal.expr, builder_.Expr(0.0f)}),
-                    eta.expr}),
                     "x")};
       }
+      default:
+        break;
+    }
+  }
 
   const auto name = GetGlslStd450FuncName(ext_opcode);
   if (name.empty()) {
@@ -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<uint32_t, 2>{0, 1}) {
+      require_named_const_def(inst, 0);
-        auto id = inst.GetSingleWordInOperand(vector_arg);
+      require_named_const_def(inst, 1);
-        auto* operand_def = GetDefInfo(id);
-        if (operand_def) {
-          operand_def->requires_named_const_def = true;
     }
+    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;
       }
     }
   }

src/reader/spirv/function_glsl_std_450_test.cc

@@ -739,7 +739,6 @@ INSTANTIATE_TEST_SUITE_P(
     ::testing::ValuesIn(std::vector<GlslStd450Case>{
         {"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