From e0ecd86e73ff9e6def9e1460cb0291f175044046 Mon Sep 17 00:00:00 2001 From: Zhaoming Jiang Date: Fri, 15 Jul 2022 11:05:29 +0000 Subject: [PATCH] tint/writer/glsl: Fix emitting float modulo with mixing type This patch fix the GLSL writer issue that emit only one helper function when using both `v % s`, `s % v` and `v % vs in the shader, where `s` is of `f32` and `v` is a vector of `f32`. Unittests are added for GLSL. Bug: tint:1614 Change-Id: Ia89ae010341b9c88b8101cc6febab7d83c96bb17 Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/96085 Reviewed-by: Ben Clayton Kokoro: Kokoro Commit-Queue: Ben Clayton --- src/tint/writer/glsl/generator_impl.cc | 4 +- src/tint/writer/glsl/generator_impl.h | 20 +++++- .../writer/glsl/generator_impl_binary_test.cc | 70 +++++++++++++++++++ 3 files changed, 92 insertions(+), 2 deletions(-) diff --git a/src/tint/writer/glsl/generator_impl.cc b/src/tint/writer/glsl/generator_impl.cc index 767fecba3e..1a2b5dc29b 100644 --- a/src/tint/writer/glsl/generator_impl.cc +++ b/src/tint/writer/glsl/generator_impl.cc @@ -500,7 +500,9 @@ bool GeneratorImpl::EmitBitwiseBoolOp(std::ostream& out, const ast::BinaryExpres bool GeneratorImpl::EmitFloatModulo(std::ostream& out, const ast::BinaryExpression* expr) { std::string fn; auto* ret_ty = TypeOf(expr)->UnwrapRef(); - fn = utils::GetOrCreate(float_modulo_funcs_, ret_ty, [&]() -> std::string { + auto* lhs_ty = TypeOf(expr->lhs)->UnwrapRef(); + auto* rhs_ty = TypeOf(expr->rhs)->UnwrapRef(); + fn = utils::GetOrCreate(float_modulo_funcs_, {lhs_ty, rhs_ty}, [&]() -> std::string { TextBuffer b; TINT_DEFER(helpers_.Append(b)); diff --git a/src/tint/writer/glsl/generator_impl.h b/src/tint/writer/glsl/generator_impl.h index 58a0e50dbe..37352c55a7 100644 --- a/src/tint/writer/glsl/generator_impl.h +++ b/src/tint/writer/glsl/generator_impl.h @@ -492,6 +492,23 @@ class GeneratorImpl : public TextGenerator { }; }; + /// The structure holding both type of two operands for a binary operator. + struct BinaryOperandType { + const sem::Type* lhs_type; + const sem::Type* rhs_type; + bool operator==(const BinaryOperandType& rhs) const { + return lhs_type == rhs.lhs_type && rhs_type == rhs.rhs_type; + } + /// Hasher is a std::hash function for BinaryOperandType + struct Hasher { + /// @param i the BinaryOperandType to hash + /// @returns the hash of `i` + inline std::size_t operator()(const BinaryOperandType& i) const { + return utils::Hash(i.lhs_type, i.rhs_type); + } + }; + }; + /// CallBuiltinHelper will call the builtin helper function, creating it /// if it hasn't been built already. If the builtin needs to be built then /// CallBuiltinHelper will generate the function signature and will call @@ -522,7 +539,8 @@ class GeneratorImpl : public TextGenerator { std::unordered_map builtins_; std::unordered_map dynamic_vector_write_; std::unordered_map int_dot_funcs_; - std::unordered_map float_modulo_funcs_; + std::unordered_map + float_modulo_funcs_; std::unordered_set emitted_structs_; bool requires_oes_sample_variables_ = false; bool requires_default_precision_qualifier_ = false; diff --git a/src/tint/writer/glsl/generator_impl_binary_test.cc b/src/tint/writer/glsl/generator_impl_binary_test.cc index b52925575d..a9ccffd5dc 100644 --- a/src/tint/writer/glsl/generator_impl_binary_test.cc +++ b/src/tint/writer/glsl/generator_impl_binary_test.cc @@ -273,6 +273,76 @@ TEST_F(GlslGeneratorImplTest_Binary, ModVec3F32) { EXPECT_EQ(out.str(), "tint_float_modulo(a, b)"); } +TEST_F(GlslGeneratorImplTest_Binary, ModVec3F32ScalarF32) { + GlobalVar("a", ty.vec3(), ast::StorageClass::kPrivate); + GlobalVar("b", ty.f32(), ast::StorageClass::kPrivate); + + auto* expr = create(ast::BinaryOp::kModulo, Expr("a"), Expr("b")); + WrapInFunction(expr); + + GeneratorImpl& gen = Build(); + + std::stringstream out; + ASSERT_TRUE(gen.EmitExpression(out, expr)) << gen.error(); + EXPECT_EQ(out.str(), "tint_float_modulo(a, b)"); +} + +TEST_F(GlslGeneratorImplTest_Binary, ModScalarF32Vec3F32) { + GlobalVar("a", ty.f32(), ast::StorageClass::kPrivate); + GlobalVar("b", ty.vec3(), ast::StorageClass::kPrivate); + + auto* expr = create(ast::BinaryOp::kModulo, Expr("a"), Expr("b")); + WrapInFunction(expr); + + GeneratorImpl& gen = Build(); + + std::stringstream out; + ASSERT_TRUE(gen.EmitExpression(out, expr)) << gen.error(); + EXPECT_EQ(out.str(), "tint_float_modulo(a, b)"); +} + +TEST_F(GlslGeneratorImplTest_Binary, ModMixedVec3ScalarF32) { + GlobalVar("a", ty.vec3(), ast::StorageClass::kPrivate); + GlobalVar("b", ty.f32(), ast::StorageClass::kPrivate); + + auto* expr_vec_mod_vec = + create(ast::BinaryOp::kModulo, Expr("a"), Expr("a")); + auto* expr_vec_mod_scalar = + create(ast::BinaryOp::kModulo, Expr("a"), Expr("b")); + auto* expr_scalar_mod_vec = + create(ast::BinaryOp::kModulo, Expr("b"), Expr("a")); + WrapInFunction(expr_vec_mod_vec, expr_vec_mod_scalar, expr_scalar_mod_vec); + + GeneratorImpl& gen = Build(); + + ASSERT_TRUE(gen.Generate()) << gen.error(); + EXPECT_EQ(gen.result(), R"(#version 310 es + +vec3 tint_float_modulo(vec3 lhs, vec3 rhs) { + return (lhs - rhs * trunc(lhs / rhs)); +} + +vec3 tint_float_modulo_1(vec3 lhs, float rhs) { + return (lhs - rhs * trunc(lhs / rhs)); +} + +vec3 tint_float_modulo_2(float lhs, vec3 rhs) { + return (lhs - rhs * trunc(lhs / rhs)); +} + + +vec3 a = vec3(0.0f, 0.0f, 0.0f); +float b = 0.0f; +layout(local_size_x = 1, local_size_y = 1, local_size_z = 1) in; +void test_function() { + vec3 tint_symbol = tint_float_modulo(a, a); + vec3 tint_symbol_1 = tint_float_modulo_1(a, b); + vec3 tint_symbol_2 = tint_float_modulo_2(b, a); + return; +} +)"); +} + TEST_F(GlslGeneratorImplTest_Binary, Logical_Multi) { // (a && b) || (c || d) GlobalVar("a", ty.bool_(), ast::StorageClass::kPrivate);