Merge pull request #3518 from wwylele/hashable-struct

Common/Hash: abstract HashableStruct from GLShader::PicaShaderConfig

src/common/hash.h

@@ -5,6 +5,7 @@
 #pragma once
 
 #include <cstddef>
+#include <cstring>
 #include "common/cityhash.h"
 #include "common/common_types.h"
 
@@ -21,16 +22,50 @@ static inline u64 ComputeHash64(const void* data, size_t len) {
 }
 
 /**
- * Computes a 64-bit hash of a struct. In addition to being POD (trivially copyable and having
+ * Computes a 64-bit hash of a struct. In addition to being trivially copyable, it is also critical
- * standard layout), it is also critical that either the struct includes no padding, or that any
+ * that either the struct includes no padding, or that any padding is initialized to a known value
- * padding is initialized to a known value by memsetting the struct to 0 before filling it in.
+ * by memsetting the struct to 0 before filling it in.
  */
 template <typename T>
 static inline u64 ComputeStructHash64(const T& data) {
-    static_assert(
+    static_assert(std::is_trivially_copyable<T>(),
-        std::is_trivially_copyable<T>::value && std::is_standard_layout<T>::value,
+                  "Type passed to ComputeStructHash64 must be trivially copyable");
-        "Type passed to ComputeStructHash64 must be trivially copyable and standard layout");
     return ComputeHash64(&data, sizeof(data));
 }
 
+/// A helper template that ensures the padding in a struct is initialized by memsetting to 0.
+template <typename T>
+struct HashableStruct {
+    // In addition to being trivially copyable, T must also have a trivial default constructor,
+    // because any member initialization would be overridden by memset
+    static_assert(std::is_trivial<T>(), "Type passed to HashableStruct must be trivial");
+    /*
+     * We use a union because "implicitly-defined copy/move constructor for a union X copies the
+     * object representation of X." and "implicitly-defined copy assignment operator for a union X
+     * copies the object representation (3.9) of X." = Bytewise copy instead of memberwise copy.
+     * This is important because the padding bytes are included in the hash and comparison between
+     * objects.
+     */
+    union {
+        T state;
+    };
+
+    HashableStruct() {
+        // Memset structure to zero padding bits, so that they will be deterministic when hashing
+        std::memset(&state, 0, sizeof(T));
+    }
+
+    bool operator==(const HashableStruct<T>& o) const {
+        return std::memcmp(&state, &o.state, sizeof(T)) == 0;
+    };
+
+    bool operator!=(const HashableStruct<T>& o) const {
+        return !(*this == o);
+    };
+
+    size_t Hash() const {
+        return Common::ComputeStructHash64(state);
+    }
+};
+
 } // namespace Common

src/video_core/renderer_opengl/gl_shader_gen.cpp

@@ -65,8 +65,6 @@ PicaShaderConfig PicaShaderConfig::BuildFromRegs(const Pica::Regs& regs) {
     PicaShaderConfig res;
 
     auto& state = res.state;
-    // Memset structure to zero padding bits, so that they will be deterministic when hashing
-    std::memset(&state, 0, sizeof(PicaShaderConfig::State));
 
     state.scissor_test_mode = regs.rasterizer.scissor_test.mode;
 

src/video_core/renderer_opengl/gl_shader_gen.h

@@ -24,6 +24,82 @@ enum Attributes {
     ATTRIBUTE_VIEW,
 };
 
+// NOTE: MSVC15 (Update 2) doesn't think `delete`'d constructors and operators are TC.
+//       This makes BitField not TC when used in a union or struct so we have to resort
+//       to this ugly hack.
+//       Once that bug is fixed we can use Pica::Regs::TevStageConfig here.
+//       Doesn't include const_color because we don't sync it, see comment in BuildFromRegs()
+struct TevStageConfigRaw {
+    u32 sources_raw;
+    u32 modifiers_raw;
+    u32 ops_raw;
+    u32 scales_raw;
+    explicit operator Pica::TexturingRegs::TevStageConfig() const noexcept {
+        Pica::TexturingRegs::TevStageConfig stage;
+        stage.sources_raw = sources_raw;
+        stage.modifiers_raw = modifiers_raw;
+        stage.ops_raw = ops_raw;
+        stage.const_color = 0;
+        stage.scales_raw = scales_raw;
+        return stage;
+    }
+};
+
+struct PicaShaderConfigState {
+    Pica::FramebufferRegs::CompareFunc alpha_test_func;
+    Pica::RasterizerRegs::ScissorMode scissor_test_mode;
+    Pica::TexturingRegs::TextureConfig::TextureType texture0_type;
+    bool texture2_use_coord1;
+    std::array<TevStageConfigRaw, 6> tev_stages;
+    u8 combiner_buffer_input;
+
+    Pica::RasterizerRegs::DepthBuffering depthmap_enable;
+    Pica::TexturingRegs::FogMode fog_mode;
+    bool fog_flip;
+
+    struct {
+        struct {
+            unsigned num;
+            bool directional;
+            bool two_sided_diffuse;
+            bool dist_atten_enable;
+            bool spot_atten_enable;
+            bool geometric_factor_0;
+            bool geometric_factor_1;
+        } light[8];
+
+        bool enable;
+        unsigned src_num;
+        Pica::LightingRegs::LightingBumpMode bump_mode;
+        unsigned bump_selector;
+        bool bump_renorm;
+        bool clamp_highlights;
+
+        Pica::LightingRegs::LightingConfig config;
+        Pica::LightingRegs::LightingFresnelSelector fresnel_selector;
+
+        struct {
+            bool enable;
+            bool abs_input;
+            Pica::LightingRegs::LightingLutInput type;
+            float scale;
+        } lut_d0, lut_d1, lut_sp, lut_fr, lut_rr, lut_rg, lut_rb;
+    } lighting;
+
+    struct {
+        bool enable;
+        u32 coord;
+        Pica::TexturingRegs::ProcTexClamp u_clamp, v_clamp;
+        Pica::TexturingRegs::ProcTexCombiner color_combiner, alpha_combiner;
+        bool separate_alpha;
+        bool noise_enable;
+        Pica::TexturingRegs::ProcTexShift u_shift, v_shift;
+        u32 lut_width;
+        u32 lut_offset;
+        Pica::TexturingRegs::ProcTexFilter lut_filter;
+    } proctex;
+};
+
 /**
  * This struct contains all state used to generate the GLSL shader program that emulates the current
  * Pica register configuration. This struct is used as a cache key for generated GLSL shader
@@ -31,13 +107,8 @@ enum Attributes {
  * directly accessing Pica registers. This should reduce the risk of bugs in shader generation where
  * Pica state is not being captured in the shader cache key, thereby resulting in (what should be)
  * two separate shaders sharing the same key.
- *
- * We use a union because "implicitly-defined copy/move constructor for a union X copies the object
- * representation of X." and "implicitly-defined copy assignment operator for a union X copies the
- * object representation (3.9) of X." = Bytewise copy instead of memberwise copy. This is important
- * because the padding bytes are included in the hash and comparison between objects.
  */
-union PicaShaderConfig {
+struct PicaShaderConfig : Common::HashableStruct<PicaShaderConfigState> {
 
     /// Construct a PicaShaderConfig with the given Pica register configuration.
     static PicaShaderConfig BuildFromRegs(const Pica::Regs& regs);
@@ -49,87 +120,6 @@ union PicaShaderConfig {
     bool TevStageUpdatesCombinerBufferAlpha(unsigned stage_index) const {
         return (stage_index < 4) && ((state.combiner_buffer_input >> 4) & (1 << stage_index));
     }
-
-    bool operator==(const PicaShaderConfig& o) const {
-        return std::memcmp(&state, &o.state, sizeof(PicaShaderConfig::State)) == 0;
-    };
-
-    // NOTE: MSVC15 (Update 2) doesn't think `delete`'d constructors and operators are TC.
-    //       This makes BitField not TC when used in a union or struct so we have to resort
-    //       to this ugly hack.
-    //       Once that bug is fixed we can use Pica::Regs::TevStageConfig here.
-    //       Doesn't include const_color because we don't sync it, see comment in BuildFromRegs()
-    struct TevStageConfigRaw {
-        u32 sources_raw;
-        u32 modifiers_raw;
-        u32 ops_raw;
-        u32 scales_raw;
-        explicit operator Pica::TexturingRegs::TevStageConfig() const noexcept {
-            Pica::TexturingRegs::TevStageConfig stage;
-            stage.sources_raw = sources_raw;
-            stage.modifiers_raw = modifiers_raw;
-            stage.ops_raw = ops_raw;
-            stage.const_color = 0;
-            stage.scales_raw = scales_raw;
-            return stage;
-        }
-    };
-
-    struct State {
-        Pica::FramebufferRegs::CompareFunc alpha_test_func;
-        Pica::RasterizerRegs::ScissorMode scissor_test_mode;
-        Pica::TexturingRegs::TextureConfig::TextureType texture0_type;
-        bool texture2_use_coord1;
-        std::array<TevStageConfigRaw, 6> tev_stages;
-        u8 combiner_buffer_input;
-
-        Pica::RasterizerRegs::DepthBuffering depthmap_enable;
-        Pica::TexturingRegs::FogMode fog_mode;
-        bool fog_flip;
-
-        struct {
-            struct {
-                unsigned num;
-                bool directional;
-                bool two_sided_diffuse;
-                bool dist_atten_enable;
-                bool spot_atten_enable;
-                bool geometric_factor_0;
-                bool geometric_factor_1;
-            } light[8];
-
-            bool enable;
-            unsigned src_num;
-            Pica::LightingRegs::LightingBumpMode bump_mode;
-            unsigned bump_selector;
-            bool bump_renorm;
-            bool clamp_highlights;
-
-            Pica::LightingRegs::LightingConfig config;
-            Pica::LightingRegs::LightingFresnelSelector fresnel_selector;
-
-            struct {
-                bool enable;
-                bool abs_input;
-                Pica::LightingRegs::LightingLutInput type;
-                float scale;
-            } lut_d0, lut_d1, lut_sp, lut_fr, lut_rr, lut_rg, lut_rb;
-        } lighting;
-
-        struct {
-            bool enable;
-            u32 coord;
-            Pica::TexturingRegs::ProcTexClamp u_clamp, v_clamp;
-            Pica::TexturingRegs::ProcTexCombiner color_combiner, alpha_combiner;
-            bool separate_alpha;
-            bool noise_enable;
-            Pica::TexturingRegs::ProcTexShift u_shift, v_shift;
-            u32 lut_width;
-            u32 lut_offset;
-            Pica::TexturingRegs::ProcTexFilter lut_filter;
-        } proctex;
-
-    } state;
 };
 
 /**
@@ -152,7 +142,7 @@ namespace std {
 template <>
 struct hash<GLShader::PicaShaderConfig> {
     size_t operator()(const GLShader::PicaShaderConfig& k) const {
-        return Common::ComputeStructHash64(k.state);
+        return k.Hash();
     }
 };
 } // namespace std