From ab10db454bbfdbcd3d17a924ed496bf020dc5b86 Mon Sep 17 00:00:00 2001
From: Ben Clayton <bclayton@google.com>
Date: Mon, 8 Feb 2021 23:07:45 +0000
Subject: [PATCH] writer/spirv: Fix intrinsic calls with ptr 'out' parameters

Use the new semantic::Intrinsic::Parameters() information to determine whether a parameter is a pointer.
Don't generate a load if the parameter expects a pointer.

Fixed: tint:361
Change-Id: I1420a6b0e22d52f67a5e52151fb073ac33df5bd5
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/40508
Commit-Queue: Ben Clayton <bclayton@google.com>
Reviewed-by: dan sinclair <dsinclair@chromium.org>
---
 src/writer/spirv/builder.cc                | 11 ++-
 src/writer/spirv/builder_intrinsic_test.cc | 90 ++++++++++++++++++++++
 2 files changed, 98 insertions(+), 3 deletions(-)

diff --git a/src/writer/spirv/builder.cc b/src/writer/spirv/builder.cc
index 386f5a98f8..6bdeab53d9 100644
--- a/src/writer/spirv/builder.cc
+++ b/src/writer/spirv/builder.cc
@@ -2010,12 +2010,17 @@ uint32_t Builder::GenerateIntrinsic(ast::CallExpression* call,
     return 0;
   }
 
-  for (auto* p : call->params()) {
-    auto val_id = GenerateExpression(p);
+  for (size_t i = 0; i < call->params().size(); i++) {
+    auto* arg = call->params()[i];
+    auto& param = intrinsic->Parameters()[i];
+    auto val_id = GenerateExpression(arg);
     if (val_id == 0) {
       return false;
     }
-    val_id = GenerateLoadIfNeeded(TypeOf(p), val_id);
+
+    if (!param.type->Is<type::Pointer>()) {
+      val_id = GenerateLoadIfNeeded(TypeOf(arg), val_id);
+    }
 
     params.emplace_back(Operand::Int(val_id));
   }
diff --git a/src/writer/spirv/builder_intrinsic_test.cc b/src/writer/spirv/builder_intrinsic_test.cc
index 9078c2f3e3..bd8e0c3e2d 100644
--- a/src/writer/spirv/builder_intrinsic_test.cc
+++ b/src/writer/spirv/builder_intrinsic_test.cc
@@ -16,12 +16,14 @@
 
 #include "gtest/gtest.h"
 #include "src/ast/call_expression.h"
+#include "src/ast/call_statement.h"
 #include "src/ast/float_literal.h"
 #include "src/ast/identifier_expression.h"
 #include "src/ast/intrinsic_texture_helper_test.h"
 #include "src/ast/member_accessor_expression.h"
 #include "src/ast/scalar_constructor_expression.h"
 #include "src/ast/sint_literal.h"
+#include "src/ast/stage_decoration.h"
 #include "src/ast/struct.h"
 #include "src/ast/struct_member.h"
 #include "src/ast/type_constructor_expression.h"
@@ -1279,6 +1281,94 @@ INSTANTIATE_TEST_SUITE_P(IntrinsicBuilderTest,
                          Intrinsic_Builtin_ThreeParam_Uint_Test,
                          testing::Values(IntrinsicData{"clamp", "UClamp"}));
 
+TEST_F(IntrinsicBuilderTest, Call_Modf) {
+  auto* out = Var("out", ast::StorageClass::kFunction, ty.vec2<f32>());
+  auto* expr = Call("modf", vec2<f32>(1.0f, 2.0f), "out");
+  Func("a_func", ast::VariableList{}, ty.void_(),
+       ast::StatementList{
+           create<ast::VariableDeclStatement>(out),
+           create<ast::CallStatement>(expr),
+       },
+       ast::FunctionDecorationList{
+           create<ast::StageDecoration>(ast::PipelineStage::kVertex),
+       });
+
+  spirv::Builder& b = Build();
+
+  ASSERT_TRUE(b.Build()) << b.error();
+  auto got = DumpBuilder(b);
+  auto* expect = R"(OpCapability Shader
+%11 = OpExtInstImport "GLSL.std.450"
+OpMemoryModel Logical GLSL450
+OpEntryPoint Vertex %3 "a_func"
+OpName %3 "a_func"
+OpName %5 "out"
+%2 = OpTypeVoid
+%1 = OpTypeFunction %2
+%8 = OpTypeFloat 32
+%7 = OpTypeVector %8 2
+%6 = OpTypePointer Function %7
+%9 = OpConstantNull %7
+%12 = OpConstant %8 1
+%13 = OpConstant %8 2
+%14 = OpConstantComposite %7 %12 %13
+%3 = OpFunction %2 None %1
+%4 = OpLabel
+%5 = OpVariable %6 Function %9
+%10 = OpExtInst %7 %11 Modf %14 %5
+OpReturn
+OpFunctionEnd
+)";
+  EXPECT_EQ(expect, got);
+
+  Validate(b);
+}
+
+TEST_F(IntrinsicBuilderTest, Call_Frexp) {
+  auto* out = Var("out", ast::StorageClass::kFunction, ty.vec2<i32>());
+  auto* expr = Call("frexp", vec2<f32>(1.0f, 2.0f), "out");
+  Func("a_func", ast::VariableList{}, ty.void_(),
+       ast::StatementList{
+           create<ast::VariableDeclStatement>(out),
+           create<ast::CallStatement>(expr),
+       },
+       ast::FunctionDecorationList{
+           create<ast::StageDecoration>(ast::PipelineStage::kVertex),
+       });
+
+  spirv::Builder& b = Build();
+
+  ASSERT_TRUE(b.Build()) << b.error();
+  auto got = DumpBuilder(b);
+  auto* expect = R"(OpCapability Shader
+%13 = OpExtInstImport "GLSL.std.450"
+OpMemoryModel Logical GLSL450
+OpEntryPoint Vertex %3 "a_func"
+OpName %3 "a_func"
+OpName %5 "out"
+%2 = OpTypeVoid
+%1 = OpTypeFunction %2
+%8 = OpTypeInt 32 1
+%7 = OpTypeVector %8 2
+%6 = OpTypePointer Function %7
+%9 = OpConstantNull %7
+%12 = OpTypeFloat 32
+%11 = OpTypeVector %12 2
+%14 = OpConstant %12 1
+%15 = OpConstant %12 2
+%16 = OpConstantComposite %11 %14 %15
+%3 = OpFunction %2 None %1
+%4 = OpLabel
+%5 = OpVariable %6 Function %9
+%10 = OpExtInst %11 %13 Frexp %16 %5
+OpReturn
+OpFunctionEnd
+)";
+  EXPECT_EQ(expect, got);
+
+  Validate(b);
+}
+
 TEST_F(IntrinsicBuilderTest, Call_Determinant) {
   auto* var = Global("var", ast::StorageClass::kPrivate, ty.mat3x3<f32>());