writer/spirv: Fix abs() on unsigned integers

GLSLstd450SAbs expects a *signed* integer. abs() of an unsigned number is now a no-op. Fixes WebGPU CTS tests: webgpu:shader,execution,robust_access_vertex:vertex_buffer_access:* Bug: tint:1194 Change-Id: I65c5e9f2f03aac0b788b9ba88c383cbec136d7c6 Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/65620 Commit-Queue: Ben Clayton <bclayton@chromium.org> Kokoro: Ben Clayton <bclayton@chromium.org> Reviewed-by: David Neto <dneto@google.com>
2021-10-01 08:41:55 +00:00 · 2021-10-01 08:41:55 +00:00 · 5e35864c1b
parent c57642cbd5
commit 5e35864c1b
7 changed files with 100 additions and 110 deletions
--- a/docs/origin-trial-changes.md
+++ b/docs/origin-trial-changes.md
@ -3,11 +3,12 @@
 ## Changes for M95

 ### New Features
+
 * The size of an array can now be defined using a non-overridable module-scope constant
 * The `num_workgroups` builtin is now supported.

 ### Fixes
-* Hex floats: issue an error when the magnitude is non-zero, and the exponent would cause
-    overflow. https://crbug.com/tint/1150 https://crbug.com/tint/1166
-* Reject identifiers beginning with an underscore.  https://crbug.com/tint/1179

+* Hex floats: now correctly errors when the magnitude is non-zero, and the exponent would cause overflow. [tint:1150](https://crbug.com/tint/1150), [tint:1166](https://crbug.com/tint/1166)
+* Identifers beginning with an underscore are now correctly rejected.  [tint:1179](https://crbug.com/tint/1179)
+* `abs()` fixed for unsigned integers on SPIR-V backend   [tint:1179](https://crbug.com/tint/1194)
--- a/src/writer/spirv/builder.cc
+++ b/src/writer/spirv/builder.cc
@ -124,12 +124,6 @@ const sem::Matrix* GetNestedMatrixType(const sem::Type* type) {

 uint32_t intrinsic_to_glsl_method(const sem::Intrinsic* intrinsic) {
  switch (intrinsic->Type()) {
-    case IntrinsicType::kAbs:
-      if (intrinsic->ReturnType()->is_float_scalar_or_vector()) {
-        return GLSLstd450FAbs;
-      } else {
-        return GLSLstd450SAbs;
-      }
    case IntrinsicType::kAcos:
      return GLSLstd450Acos;
    case IntrinsicType::kAsin:
@ -2338,8 +2332,17 @@ uint32_t Builder::GenerateIntrinsic(ast::CallExpression* call,
  };

  OperandList params = {Operand::Int(result_type_id), result};
-
  spv::Op op = spv::Op::OpNop;
+
+  // Pushes the parameters for a GlslStd450 extended instruction, and sets op
+  // to OpExtInst.
+  auto glsl_std450 = [&](uint32_t inst_id) {
+    auto set_id = GetGLSLstd450Import();
+    params.push_back(Operand::Int(set_id));
+    params.push_back(Operand::Int(inst_id));
+    op = spv::Op::OpExtInst;
+  };
+
  switch (intrinsic->Type()) {
    case IntrinsicType::kAny:
      op = spv::Op::OpAny;
@ -2611,18 +2614,25 @@ uint32_t Builder::GenerateIntrinsic(ast::CallExpression* call,
    case IntrinsicType::kTranspose:
      op = spv::Op::OpTranspose;
      break;
+    case IntrinsicType::kAbs:
+      if (intrinsic->ReturnType()->is_unsigned_scalar_or_vector()) {
+        // abs() only operates on *signed* integers.
+        // This is a no-op for unsigned integers.
+        return get_param_as_value_id(0);
+      }
+      if (intrinsic->ReturnType()->is_float_scalar_or_vector()) {
+        glsl_std450(GLSLstd450FAbs);
+      } else {
+        glsl_std450(GLSLstd450SAbs);
+      }
+      break;
    default: {
-      auto set_id = GetGLSLstd450Import();
      auto inst_id = intrinsic_to_glsl_method(intrinsic);
      if (inst_id == 0) {
        error_ = "unknown method " + std::string(intrinsic->str());
        return 0;
      }
-
-      params.push_back(Operand::Int(set_id));
-      params.push_back(Operand::Int(inst_id));
-
-      op = spv::Op::OpExtInst;
+      glsl_std450(inst_id);
      break;
    }
  }
--- a/src/writer/spirv/builder_intrinsic_test.cc
+++ b/src/writer/spirv/builder_intrinsic_test.cc
@ -1133,12 +1133,10 @@ INSTANTIATE_TEST_SUITE_P(IntrinsicBuilderTest,
                         Intrinsic_Builtin_SingleParam_Sint_Test,
                         testing::Values(IntrinsicData{"abs", "SAbs"}));

-using Intrinsic_Builtin_SingleParam_Uint_Test =
-    IntrinsicBuilderTestWithParam<IntrinsicData>;
-TEST_P(Intrinsic_Builtin_SingleParam_Uint_Test, Call_Scalar) {
-  auto param = GetParam();
-
-  auto* expr = Call(param.name, 1u);
+// Calling abs() on an unsigned integer scalar / vector is a no-op.
+using Intrinsic_Builtin_Abs_Uint_Test = IntrinsicBuilderTest;
+TEST_F(Intrinsic_Builtin_Abs_Uint_Test, Call_Scalar) {
+  auto* expr = Call("abs", 1u);
  WrapInFunction(expr);

  auto* func = Func("a_func", ast::VariableList{}, ty.void_(),
@ -1148,26 +1146,21 @@ TEST_P(Intrinsic_Builtin_SingleParam_Uint_Test, Call_Scalar) {

  ASSERT_TRUE(b.GenerateFunction(func)) << b.error();

-  EXPECT_EQ(b.GenerateCallExpression(expr), 5u) << b.error();
-  EXPECT_EQ(DumpBuilder(b), R"(%7 = OpExtInstImport "GLSL.std.450"
-OpName %3 "a_func"
+  EXPECT_EQ(b.GenerateCallExpression(expr), 7u) << b.error();
+  EXPECT_EQ(DumpBuilder(b), R"(OpName %3 "a_func"
 %2 = OpTypeVoid
 %1 = OpTypeFunction %2
 %6 = OpTypeInt 32 0
-%8 = OpConstant %6 1
+%7 = OpConstant %6 1
 %3 = OpFunction %2 None %1
 %4 = OpLabel
-%5 = OpExtInst %6 %7 )" + param.op +
-                                R"( %8
 OpReturn
 OpFunctionEnd
 )");
 }

-TEST_P(Intrinsic_Builtin_SingleParam_Uint_Test, Call_Vector) {
-  auto param = GetParam();
-
-  auto* expr = Call(param.name, vec2<u32>(1u, 1u));
+TEST_F(Intrinsic_Builtin_Abs_Uint_Test, Call_Vector) {
+  auto* expr = Call("abs", vec2<u32>(1u, 1u));
  WrapInFunction(expr);

  auto* func = Func("a_func", ast::VariableList{}, ty.void_(),
@ -1177,26 +1170,20 @@ TEST_P(Intrinsic_Builtin_SingleParam_Uint_Test, Call_Vector) {

  ASSERT_TRUE(b.GenerateFunction(func)) << b.error();

-  EXPECT_EQ(b.GenerateCallExpression(expr), 5u) << b.error();
-  EXPECT_EQ(DumpBuilder(b), R"(%8 = OpExtInstImport "GLSL.std.450"
-OpName %3 "a_func"
+  EXPECT_EQ(b.GenerateCallExpression(expr), 9u) << b.error();
+  EXPECT_EQ(DumpBuilder(b), R"(OpName %3 "a_func"
 %2 = OpTypeVoid
 %1 = OpTypeFunction %2
 %7 = OpTypeInt 32 0
 %6 = OpTypeVector %7 2
-%9 = OpConstant %7 1
-%10 = OpConstantComposite %6 %9 %9
+%8 = OpConstant %7 1
+%9 = OpConstantComposite %6 %8 %8
 %3 = OpFunction %2 None %1
 %4 = OpLabel
-%5 = OpExtInst %6 %8 )" + param.op +
-                                R"( %10
 OpReturn
 OpFunctionEnd
 )");
 }
-INSTANTIATE_TEST_SUITE_P(IntrinsicBuilderTest,
-                         Intrinsic_Builtin_SingleParam_Uint_Test,
-                         testing::Values(IntrinsicData{"abs", "SAbs"}));

 using Intrinsic_Builtin_DualParam_SInt_Test =
    IntrinsicBuilderTestWithParam<IntrinsicData>;
--- a/test/intrinsics/gen/abs/1ce782.wgsl.expected.spvasm
+++ b/test/intrinsics/gen/abs/1ce782.wgsl.expected.spvasm
@ -1,10 +1,9 @@
 ; SPIR-V
 ; Version: 1.3
 ; Generator: Google Tint Compiler; 0
-; Bound: 34
+; Bound: 33
 ; Schema: 0
               OpCapability Shader
-         %16 = OpExtInstImport "GLSL.std.450"
               OpMemoryModel Logical GLSL450
               OpEntryPoint Vertex %vertex_main "vertex_main" %value %vertex_point_size
               OpEntryPoint Fragment %fragment_main "fragment_main"
@ -33,36 +32,35 @@
          %9 = OpTypeFunction %void
       %uint = OpTypeInt 32 0
     %v4uint = OpTypeVector %uint 4
-         %17 = OpConstantNull %v4uint
+         %16 = OpConstantNull %v4uint
 %_ptr_Function_v4uint = OpTypePointer Function %v4uint
-         %20 = OpTypeFunction %v4float
+         %19 = OpTypeFunction %v4float
    %float_1 = OpConstant %float 1
 %abs_1ce782 = OpFunction %void None %9
         %12 = OpLabel
-        %res = OpVariable %_ptr_Function_v4uint Function %17
-         %13 = OpExtInst %v4uint %16 SAbs %17
-               OpStore %res %13
+        %res = OpVariable %_ptr_Function_v4uint Function %16
+               OpStore %res %16
               OpReturn
               OpFunctionEnd
-%vertex_main_inner = OpFunction %v4float None %20
-         %22 = OpLabel
-         %23 = OpFunctionCall %void %abs_1ce782
+%vertex_main_inner = OpFunction %v4float None %19
+         %21 = OpLabel
+         %22 = OpFunctionCall %void %abs_1ce782
               OpReturnValue %5
               OpFunctionEnd
 %vertex_main = OpFunction %void None %9
-         %25 = OpLabel
-         %26 = OpFunctionCall %v4float %vertex_main_inner
-               OpStore %value %26
+         %24 = OpLabel
+         %25 = OpFunctionCall %v4float %vertex_main_inner
+               OpStore %value %25
               OpStore %vertex_point_size %float_1
               OpReturn
               OpFunctionEnd
 %fragment_main = OpFunction %void None %9
-         %29 = OpLabel
-         %30 = OpFunctionCall %void %abs_1ce782
+         %28 = OpLabel
+         %29 = OpFunctionCall %void %abs_1ce782
               OpReturn
               OpFunctionEnd
 %compute_main = OpFunction %void None %9
-         %32 = OpLabel
-         %33 = OpFunctionCall %void %abs_1ce782
+         %31 = OpLabel
+         %32 = OpFunctionCall %void %abs_1ce782
               OpReturn
               OpFunctionEnd
--- a/test/intrinsics/gen/abs/467cd1.wgsl.expected.spvasm
+++ b/test/intrinsics/gen/abs/467cd1.wgsl.expected.spvasm
@ -1,10 +1,9 @@
 ; SPIR-V
 ; Version: 1.3
 ; Generator: Google Tint Compiler; 0
-; Bound: 34
+; Bound: 33
 ; Schema: 0
               OpCapability Shader
-         %15 = OpExtInstImport "GLSL.std.450"
               OpMemoryModel Logical GLSL450
               OpEntryPoint Vertex %vertex_main "vertex_main" %value %vertex_point_size
               OpEntryPoint Fragment %fragment_main "fragment_main"
@ -34,35 +33,34 @@
       %uint = OpTypeInt 32 0
     %uint_1 = OpConstant %uint 1
 %_ptr_Function_uint = OpTypePointer Function %uint
-         %19 = OpConstantNull %uint
-         %20 = OpTypeFunction %v4float
+         %18 = OpConstantNull %uint
+         %19 = OpTypeFunction %v4float
    %float_1 = OpConstant %float 1
 %abs_467cd1 = OpFunction %void None %9
         %12 = OpLabel
-        %res = OpVariable %_ptr_Function_uint Function %19
-         %13 = OpExtInst %uint %15 SAbs %uint_1
-               OpStore %res %13
+        %res = OpVariable %_ptr_Function_uint Function %18
+               OpStore %res %uint_1
               OpReturn
               OpFunctionEnd
-%vertex_main_inner = OpFunction %v4float None %20
-         %22 = OpLabel
-         %23 = OpFunctionCall %void %abs_467cd1
+%vertex_main_inner = OpFunction %v4float None %19
+         %21 = OpLabel
+         %22 = OpFunctionCall %void %abs_467cd1
               OpReturnValue %5
               OpFunctionEnd
 %vertex_main = OpFunction %void None %9
-         %25 = OpLabel
-         %26 = OpFunctionCall %v4float %vertex_main_inner
-               OpStore %value %26
+         %24 = OpLabel
+         %25 = OpFunctionCall %v4float %vertex_main_inner
+               OpStore %value %25
               OpStore %vertex_point_size %float_1
               OpReturn
               OpFunctionEnd
 %fragment_main = OpFunction %void None %9
-         %29 = OpLabel
-         %30 = OpFunctionCall %void %abs_467cd1
+         %28 = OpLabel
+         %29 = OpFunctionCall %void %abs_467cd1
               OpReturn
               OpFunctionEnd
 %compute_main = OpFunction %void None %9
-         %32 = OpLabel
-         %33 = OpFunctionCall %void %abs_467cd1
+         %31 = OpLabel
+         %32 = OpFunctionCall %void %abs_467cd1
               OpReturn
               OpFunctionEnd
--- a/test/intrinsics/gen/abs/7326de.wgsl.expected.spvasm
+++ b/test/intrinsics/gen/abs/7326de.wgsl.expected.spvasm
@ -1,10 +1,9 @@
 ; SPIR-V
 ; Version: 1.3
 ; Generator: Google Tint Compiler; 0
-; Bound: 34
+; Bound: 33
 ; Schema: 0
               OpCapability Shader
-         %16 = OpExtInstImport "GLSL.std.450"
               OpMemoryModel Logical GLSL450
               OpEntryPoint Vertex %vertex_main "vertex_main" %value %vertex_point_size
               OpEntryPoint Fragment %fragment_main "fragment_main"
@ -33,36 +32,35 @@
          %9 = OpTypeFunction %void
       %uint = OpTypeInt 32 0
     %v3uint = OpTypeVector %uint 3
-         %17 = OpConstantNull %v3uint
+         %16 = OpConstantNull %v3uint
 %_ptr_Function_v3uint = OpTypePointer Function %v3uint
-         %20 = OpTypeFunction %v4float
+         %19 = OpTypeFunction %v4float
    %float_1 = OpConstant %float 1
 %abs_7326de = OpFunction %void None %9
         %12 = OpLabel
-        %res = OpVariable %_ptr_Function_v3uint Function %17
-         %13 = OpExtInst %v3uint %16 SAbs %17
-               OpStore %res %13
+        %res = OpVariable %_ptr_Function_v3uint Function %16
+               OpStore %res %16
               OpReturn
               OpFunctionEnd
-%vertex_main_inner = OpFunction %v4float None %20
-         %22 = OpLabel
-         %23 = OpFunctionCall %void %abs_7326de
+%vertex_main_inner = OpFunction %v4float None %19
+         %21 = OpLabel
+         %22 = OpFunctionCall %void %abs_7326de
               OpReturnValue %5
               OpFunctionEnd
 %vertex_main = OpFunction %void None %9
-         %25 = OpLabel
-         %26 = OpFunctionCall %v4float %vertex_main_inner
-               OpStore %value %26
+         %24 = OpLabel
+         %25 = OpFunctionCall %v4float %vertex_main_inner
+               OpStore %value %25
               OpStore %vertex_point_size %float_1
               OpReturn
               OpFunctionEnd
 %fragment_main = OpFunction %void None %9
-         %29 = OpLabel
-         %30 = OpFunctionCall %void %abs_7326de
+         %28 = OpLabel
+         %29 = OpFunctionCall %void %abs_7326de
               OpReturn
               OpFunctionEnd
 %compute_main = OpFunction %void None %9
-         %32 = OpLabel
-         %33 = OpFunctionCall %void %abs_7326de
+         %31 = OpLabel
+         %32 = OpFunctionCall %void %abs_7326de
               OpReturn
               OpFunctionEnd
--- a/test/intrinsics/gen/abs/7f28e6.wgsl.expected.spvasm
+++ b/test/intrinsics/gen/abs/7f28e6.wgsl.expected.spvasm
@ -1,10 +1,9 @@
 ; SPIR-V
 ; Version: 1.3
 ; Generator: Google Tint Compiler; 0
-; Bound: 34
+; Bound: 33
 ; Schema: 0
               OpCapability Shader
-         %16 = OpExtInstImport "GLSL.std.450"
               OpMemoryModel Logical GLSL450
               OpEntryPoint Vertex %vertex_main "vertex_main" %value %vertex_point_size
               OpEntryPoint Fragment %fragment_main "fragment_main"
@ -33,36 +32,35 @@
          %9 = OpTypeFunction %void
       %uint = OpTypeInt 32 0
     %v2uint = OpTypeVector %uint 2
-         %17 = OpConstantNull %v2uint
+         %16 = OpConstantNull %v2uint
 %_ptr_Function_v2uint = OpTypePointer Function %v2uint
-         %20 = OpTypeFunction %v4float
+         %19 = OpTypeFunction %v4float
    %float_1 = OpConstant %float 1
 %abs_7f28e6 = OpFunction %void None %9
         %12 = OpLabel
-        %res = OpVariable %_ptr_Function_v2uint Function %17
-         %13 = OpExtInst %v2uint %16 SAbs %17
-               OpStore %res %13
+        %res = OpVariable %_ptr_Function_v2uint Function %16
+               OpStore %res %16
               OpReturn
               OpFunctionEnd
-%vertex_main_inner = OpFunction %v4float None %20
-         %22 = OpLabel
-         %23 = OpFunctionCall %void %abs_7f28e6
+%vertex_main_inner = OpFunction %v4float None %19
+         %21 = OpLabel
+         %22 = OpFunctionCall %void %abs_7f28e6
               OpReturnValue %5
               OpFunctionEnd
 %vertex_main = OpFunction %void None %9
-         %25 = OpLabel
-         %26 = OpFunctionCall %v4float %vertex_main_inner
-               OpStore %value %26
+         %24 = OpLabel
+         %25 = OpFunctionCall %v4float %vertex_main_inner
+               OpStore %value %25
               OpStore %vertex_point_size %float_1
               OpReturn
               OpFunctionEnd
 %fragment_main = OpFunction %void None %9
-         %29 = OpLabel
-         %30 = OpFunctionCall %void %abs_7f28e6
+         %28 = OpLabel
+         %29 = OpFunctionCall %void %abs_7f28e6
               OpReturn
               OpFunctionEnd
 %compute_main = OpFunction %void None %9
-         %32 = OpLabel
-         %33 = OpFunctionCall %void %abs_7f28e6
+         %31 = OpLabel
+         %32 = OpFunctionCall %void %abs_7f28e6
               OpReturn
               OpFunctionEnd