tint/writer: Replace use of strings for cache keys

Concatenating strings to use for cache keys is horribly inefficent and very error prone.

Add a UnorderedKeyWrapper helper to allow types to be used as a unordered_map and unordered_set key. Use this for the type_constructor_to_id_ map.

Produces SPIR-V with some duplicate SPIR-V instructions for constructors removed.

Change-Id: Ib072d485ca28bb07f03e979c133cdce1f69ee482
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/88300
Kokoro: Kokoro <noreply+kokoro@google.com>
Commit-Queue: Ben Clayton <bclayton@google.com>
Reviewed-by: Dan Sinclair <dsinclair@chromium.org>

src/tint/utils/hash.h

@@ -18,6 +18,7 @@
 #include <stdint.h>
 #include <cstdio>
 #include <functional>
+#include <utility>
 #include <vector>
 
 namespace tint::utils {
@@ -76,6 +77,53 @@ size_t Hash(const ARGS&... args) {
   return hash;
 }
 
+/// Wrapper for a hashable type enabling the wrapped value to be used as a key
+/// for an unordered_map or unordered_set.
+template <typename T>
+struct UnorderedKeyWrapper {
+  /// The wrapped value
+  const T value;
+  /// The hash of value
+  const size_t hash;
+
+  /// Constructor
+  /// @param v the value to wrap
+  explicit UnorderedKeyWrapper(const T& v) : value(v), hash(Hash(v)) {}
+
+  /// Move constructor
+  /// @param v the value to wrap
+  explicit UnorderedKeyWrapper(T&& v)
+      : value(std::move(v)), hash(Hash(value)) {}
+
+  /// @returns true if this wrapper comes before other
+  /// @param other the RHS of the operator
+  bool operator<(const UnorderedKeyWrapper& other) const {
+    return hash < other.hash;
+  }
+
+  /// @returns true if this wrapped value is equal to the other wrapped value
+  /// @param other the RHS of the operator
+  bool operator==(const UnorderedKeyWrapper& other) const {
+    return value == other.value;
+  }
+};
+
 }  // namespace tint::utils
 
+namespace std {
+
+/// Custom std::hash specialization for tint::utils::UnorderedKeyWrapper
+template <typename T>
+class hash<tint::utils::UnorderedKeyWrapper<T>> {
+ public:
+  /// @param w the UnorderedKeyWrapper
+  /// @return the hash value
+  inline std::size_t operator()(
+      const tint::utils::UnorderedKeyWrapper<T>& w) const {
+    return w.hash;
+  }
+};
+
+}  // namespace std
+
 #endif  // SRC_TINT_UTILS_HASH_H_

src/tint/utils/hash_test.cc

@@ -15,6 +15,7 @@
 #include "src/tint/utils/hash.h"
 
 #include <string>
+#include <unordered_map>
 
 #include "gtest/gtest.h"
 
@@ -43,5 +44,30 @@ TEST(HashTests, Vector) {
             Hash(std::vector<int>({1, 2, 3, 4})));
 }
 
+TEST(HashTests, UnorderedKeyWrapper) {
+  using W = UnorderedKeyWrapper<std::vector<int>>;
+
+  std::unordered_map<W, int> m;
+
+  m.emplace(W{{1, 2}}, -1);
+  EXPECT_EQ(m.size(), 1u);
+  EXPECT_EQ(m[W({1, 2})], -1);
+
+  m.emplace(W{{3, 2}}, 1);
+  EXPECT_EQ(m.size(), 2u);
+  EXPECT_EQ(m[W({3, 2})], 1);
+  EXPECT_EQ(m[W({1, 2})], -1);
+
+  m.emplace(W{{100}}, 100);
+  EXPECT_EQ(m.size(), 3u);
+  EXPECT_EQ(m[W({100})], 100);
+  EXPECT_EQ(m[W({3, 2})], 1);
+  EXPECT_EQ(m[W({1, 2})], -1);
+
+  // Reversed vector element order
+  EXPECT_EQ(m[W({2, 3})], 0);
+  EXPECT_EQ(m[W({2, 1})], 0);
+}
+
 }  // namespace
 }  // namespace tint::utils

src/tint/writer/spirv/builder.cc

@@ -1346,9 +1346,6 @@ uint32_t Builder::GenerateTypeConstructorOrConversion(
     return GenerateConstantNullIfNeeded(result_type->UnwrapRef());
   }
 
-  std::ostringstream out;
-  out << "__const_" << result_type->FriendlyName(builder_.Symbols()) << "_";
-
   result_type = result_type->UnwrapRef();
   bool constructor_is_const = IsConstructorConst(call->Declaration());
   if (has_error()) {
@@ -1386,6 +1383,12 @@ uint32_t Builder::GenerateTypeConstructorOrConversion(
   }
 
   OperandList ops;
+  static constexpr size_t kOpsResultIdx = 1;
+  static constexpr size_t kOpsFirstValueIdx = 2;
+  ops.reserve(8);
+  ops.push_back(Operand::Int(type_id));
+  ops.push_back(Operand::Int(0));  // Placeholder for the result ID
+
   for (auto* e : args) {
     uint32_t id = 0;
     id = GenerateExpressionWithLoadIfNeeded(e);
@@ -1399,8 +1402,6 @@ uint32_t Builder::GenerateTypeConstructorOrConversion(
     // value type is a correctly sized vector so we can just use it directly.
     if (result_type == value_type || result_type->Is<sem::Matrix>() ||
         result_type->Is<sem::Array>() || result_type->Is<sem::Struct>()) {
-      out << "_" << id;
-
       ops.push_back(Operand::Int(id));
       continue;
     }
@@ -1410,7 +1411,6 @@ uint32_t Builder::GenerateTypeConstructorOrConversion(
     if (value_type->is_scalar() && result_type->is_scalar()) {
       id = GenerateCastOrCopyOrPassthrough(result_type, args[0]->Declaration(),
                                            global_var);
-      out << "_" << id;
       ops.push_back(Operand::Int(id));
       continue;
     }
@@ -1461,7 +1461,6 @@ uint32_t Builder::GenerateTypeConstructorOrConversion(
           result_is_spec_composite = true;
         }
 
-        out << "_" << extract_id;
         ops.push_back(Operand::Int(extract_id));
       }
     } else {
@@ -1476,33 +1475,27 @@ uint32_t Builder::GenerateTypeConstructorOrConversion(
       args[0]->Type()->UnwrapRef()->is_scalar()) {
     size_t vec_size = init_result_type->As<sem::Vector>()->Width();
     for (size_t i = 0; i < (vec_size - 1); ++i) {
-      ops.push_back(ops[0]);
+      ops.push_back(ops[kOpsFirstValueIdx]);
     }
   }
 
-  auto str = out.str();
+  return utils::GetOrCreate(
-  auto val = type_constructor_to_id_.find(str);
+      type_constructor_to_id_, OperandListKey{ops}, [&]() -> uint32_t {
-  if (val != type_constructor_to_id_.end()) {
+        auto result = result_op();
-    return val->second;
+        ops[kOpsResultIdx] = result;
-  }
 
-  auto result = result_op();
+        if (result_is_spec_composite) {
-  ops.insert(ops.begin(), result);
+          push_type(spv::Op::OpSpecConstantComposite, ops);
-  ops.insert(ops.begin(), Operand::Int(type_id));
+        } else if (result_is_constant_composite) {
+          push_type(spv::Op::OpConstantComposite, ops);
+        } else {
+          if (!push_function_inst(spv::Op::OpCompositeConstruct, ops)) {
+            return 0;
+          }
+        }
 
-  type_constructor_to_id_[str] = result.to_i();
+        return result.to_i();
-
+      });
-  if (result_is_spec_composite) {
-    push_type(spv::Op::OpSpecConstantComposite, ops);
-  } else if (result_is_constant_composite) {
-    push_type(spv::Op::OpConstantComposite, ops);
-  } else {
-    if (!push_function_inst(spv::Op::OpCompositeConstruct, ops)) {
-      return 0;
-    }
-  }
-
-  return result.to_i();
 }
 
 uint32_t Builder::GenerateCastOrCopyOrPassthrough(

src/tint/writer/spirv/builder.h

@@ -604,7 +604,7 @@ class Builder {
   std::unordered_map<sem::CallTargetSignature, uint32_t> func_sig_to_id_;
   std::unordered_map<const sem::Type*, uint32_t> type_to_id_;
   std::unordered_map<ScalarConstant, uint32_t> const_to_id_;
-  std::unordered_map<std::string, uint32_t> type_constructor_to_id_;
+  std::unordered_map<OperandListKey, uint32_t> type_constructor_to_id_;
   std::unordered_map<const sem::Type*, uint32_t> const_null_to_id_;
   std::unordered_map<uint64_t, uint32_t> const_splat_to_id_;
   std::unordered_map<const sem::Type*, uint32_t>

src/tint/writer/spirv/operand.cc

@@ -19,21 +19,21 @@ namespace tint::writer::spirv {
 // static
 Operand Operand::Float(float val) {
   Operand o(Kind::kFloat);
-  o.set_float(val);
+  o.float_val_ = val;
   return o;
 }
 
 // static
 Operand Operand::Int(uint32_t val) {
   Operand o(Kind::kInt);
-  o.set_int(val);
+  o.int_val_ = val;
   return o;
 }
 
 // static
 Operand Operand::String(const std::string& val) {
   Operand o(Kind::kString);
-  o.set_string(val);
+  o.str_val_ = val;
   return o;
 }
 

src/tint/writer/spirv/operand.h

@@ -15,9 +15,12 @@
 #ifndef SRC_TINT_WRITER_SPIRV_OPERAND_H_
 #define SRC_TINT_WRITER_SPIRV_OPERAND_H_
 
+#include <cstring>
 #include <string>
 #include <vector>
 
+#include "src/tint/utils/hash.h"
+
 namespace tint::writer::spirv {
 
 /// A single SPIR-V instruction operand
@@ -53,16 +56,29 @@ class Operand {
   /// @returns a copy of this operand
   Operand& operator=(const Operand& b) = default;
 
-  /// Sets the float value
+  /// Equality operator
-  /// @param val the value to set
+  /// @param other the RHS of the operator
-  void set_float(float val) { float_val_ = val; }
+  /// @returns true if this operand is equal to other
-  /// Sets the int value
+  bool operator==(const Operand& other) const {
-  /// @param val the value to set
+    if (kind_ == other.kind_) {
-  void set_int(uint32_t val) { int_val_ = val; }
+      switch (kind_) {
-  /// Sets the string value
+        case tint::writer::spirv::Operand::Kind::kFloat:
-  /// @param val the value to set
+          // Use memcmp to work around:
-  void set_string(const std::string& val) { str_val_ = val; }
+          // error: comparing floating point with == or != is unsafe
+          // [-Werror,-Wfloat-equal]
+          return memcmp(&float_val_, &other.float_val_, sizeof(float_val_)) ==
+                 0;
+        case tint::writer::spirv::Operand::Kind::kInt:
+          return int_val_ == other.int_val_;
+        case tint::writer::spirv::Operand::Kind::kString:
+          return str_val_ == other.str_val_;
+      }
+    }
+    return false;
+  }
 
+  /// @returns the kind of the operand
+  Kind GetKind() const { return kind_; }
   /// @returns true if this is a float operand
   bool IsFloat() const { return kind_ == Kind::kFloat; }
   /// @returns true if this is an integer operand
@@ -90,6 +106,30 @@ class Operand {
 /// A list of operands
 using OperandList = std::vector<Operand>;
 
+using OperandListKey = utils::UnorderedKeyWrapper<OperandList>;
+
 }  // namespace tint::writer::spirv
 
+namespace std {
+
+/// Custom std::hash specialization for tint::writer::spirv::Operand
+template <>
+class hash<tint::writer::spirv::Operand> {
+ public:
+  /// @param o the Operand
+  /// @return the hash value
+  inline std::size_t operator()(const tint::writer::spirv::Operand& o) const {
+    switch (o.GetKind()) {
+      case tint::writer::spirv::Operand::Kind::kFloat:
+        return tint::utils::Hash(o.to_f());
+      case tint::writer::spirv::Operand::Kind::kInt:
+        return tint::utils::Hash(o.to_i());
+      case tint::writer::spirv::Operand::Kind::kString:
+        return tint::utils::Hash(o.to_s());
+    }
+    return 0;
+  }
+};
+
+}  // namespace std
 #endif  // SRC_TINT_WRITER_SPIRV_OPERAND_H_

test/tint/types/function_scope_var_conversions.wgsl.expected.spvasm

@@ -1,7 +1,7 @@
 ; SPIR-V
 ; Version: 1.3
 ; Generator: Google Tint Compiler; 0
-; Bound: 73
+; Bound: 72
 ; Schema: 0
                OpCapability Shader
                OpMemoryModel Logical GLSL450
@@ -69,12 +69,11 @@
 %_ptr_Function_v3uint = OpTypePointer Function %v3uint
          %61 = OpConstantNull %v3uint
          %63 = OpConstantComposite %v3uint %uint_1 %uint_1 %uint_1
-         %65 = OpConstantComposite %v3bool %true %true %true
      %v4bool = OpTypeVector %bool 4
       %false = OpConstantFalse %bool
-         %69 = OpConstantComposite %v4bool %true %false %true %false
+         %68 = OpConstantComposite %v4bool %true %false %true %false
 %_ptr_Function_v4bool = OpTypePointer Function %v4bool
-         %72 = OpConstantNull %v4bool
+         %71 = OpConstantNull %v4bool
 %constant_with_non_constant = OpFunction %void None %1
           %4 = OpLabel
           %a = OpVariable %_ptr_Function_float Function %6
@@ -107,7 +106,7 @@
  %v3u32_var2 = OpVariable %_ptr_Function_v3uint Function %61
  %v3u32_var3 = OpVariable %_ptr_Function_v3uint Function %61
 %v3bool_var4 = OpVariable %_ptr_Function_v3bool Function %45
-%v4bool_var5 = OpVariable %_ptr_Function_v4bool Function %72
+%v4bool_var5 = OpVariable %_ptr_Function_v4bool Function %71
                OpStore %bool_var1 %true
                OpStore %bool_var2 %true
                OpStore %bool_var3 %true
@@ -127,7 +126,7 @@
                OpStore %v3u32_var1 %58
                OpStore %v3u32_var2 %58
                OpStore %v3u32_var3 %63
-               OpStore %v3bool_var4 %65
+               OpStore %v3bool_var4 %42
-               OpStore %v4bool_var5 %69
+               OpStore %v4bool_var5 %68
                OpReturn
                OpFunctionEnd

test/tint/types/module_scope_var_conversions.wgsl.expected.spvasm

@@ -1,7 +1,7 @@
 ; SPIR-V
 ; Version: 1.3
 ; Generator: Google Tint Compiler; 0
-; Bound: 55
+; Bound: 54
 ; Schema: 0
                OpCapability Shader
                OpMemoryModel Logical GLSL450
@@ -64,43 +64,42 @@
  %v3u32_var1 = OpVariable %_ptr_Private_v3uint Private %32
  %v3u32_var2 = OpVariable %_ptr_Private_v3uint Private %32
  %v3u32_var3 = OpVariable %_ptr_Private_v3uint Private %32
-         %37 = OpConstantComposite %v3bool %true %true %true
+%v3bool_var4 = OpVariable %_ptr_Private_v3bool Private %20
-%v3bool_var4 = OpVariable %_ptr_Private_v3bool Private %37
      %v4bool = OpTypeVector %bool 4
       %false = OpConstantFalse %bool
-         %41 = OpConstantComposite %v4bool %true %false %true %false
+         %40 = OpConstantComposite %v4bool %true %false %true %false
 %_ptr_Private_v4bool = OpTypePointer Private %v4bool
-%v4bool_var5 = OpVariable %_ptr_Private_v4bool Private %41
+%v4bool_var5 = OpVariable %_ptr_Private_v4bool Private %40
        %void = OpTypeVoid
-         %44 = OpTypeFunction %void
+         %43 = OpTypeFunction %void
-         %48 = OpConstantNull %bool
+         %47 = OpConstantNull %bool
-         %49 = OpConstantNull %int
+         %48 = OpConstantNull %int
-         %50 = OpConstantNull %uint
+         %49 = OpConstantNull %uint
-         %51 = OpConstantNull %v3bool
+         %50 = OpConstantNull %v3bool
-         %52 = OpConstantNull %v4bool
+         %51 = OpConstantNull %v4bool
-         %53 = OpConstantNull %v3int
+         %52 = OpConstantNull %v3int
-         %54 = OpConstantNull %v3uint
+         %53 = OpConstantNull %v3uint
-       %main = OpFunction %void None %44
+       %main = OpFunction %void None %43
-         %47 = OpLabel
+         %46 = OpLabel
-               OpStore %bool_var1 %48
+               OpStore %bool_var1 %47
-               OpStore %bool_var2 %48
+               OpStore %bool_var2 %47
-               OpStore %bool_var3 %48
+               OpStore %bool_var3 %47
-               OpStore %i32_var1 %49
+               OpStore %i32_var1 %48
-               OpStore %i32_var2 %49
+               OpStore %i32_var2 %48
-               OpStore %i32_var3 %49
+               OpStore %i32_var3 %48
-               OpStore %u32_var1 %50
+               OpStore %u32_var1 %49
-               OpStore %u32_var2 %50
+               OpStore %u32_var2 %49
-               OpStore %u32_var3 %50
+               OpStore %u32_var3 %49
-               OpStore %v3bool_var1 %51
+               OpStore %v3bool_var1 %50
-               OpStore %v3bool_var2 %51
+               OpStore %v3bool_var2 %50
-               OpStore %v3bool_var3 %51
+               OpStore %v3bool_var3 %50
-               OpStore %v3bool_var4 %51
+               OpStore %v3bool_var4 %50
-               OpStore %v4bool_var5 %52
+               OpStore %v4bool_var5 %51
-               OpStore %v3i32_var1 %53
+               OpStore %v3i32_var1 %52
-               OpStore %v3i32_var2 %53
+               OpStore %v3i32_var2 %52
-               OpStore %v3i32_var3 %53
+               OpStore %v3i32_var3 %52
-               OpStore %v3u32_var1 %54
+               OpStore %v3u32_var1 %53
-               OpStore %v3u32_var2 %54
+               OpStore %v3u32_var2 %53
-               OpStore %v3u32_var3 %54
+               OpStore %v3u32_var3 %53
                OpReturn
                OpFunctionEnd