From 30cc8c10cd958fd88192fe09d86a59f9eddd9d81 Mon Sep 17 00:00:00 2001 From: wwylele Date: Tue, 13 Mar 2018 23:39:28 +0200 Subject: [PATCH 1/2] Common/Hash: abstract HashableStruct from GLShader::PicaShaderConfig --- src/common/hash.h | 33 ++++ .../renderer_opengl/gl_shader_gen.cpp | 2 - .../renderer_opengl/gl_shader_gen.h | 166 ++++++++---------- 3 files changed, 111 insertions(+), 90 deletions(-) diff --git a/src/common/hash.h b/src/common/hash.h index cc3cece682..042f4e5169 100644 --- a/src/common/hash.h +++ b/src/common/hash.h @@ -5,6 +5,7 @@ #pragma once #include +#include #include "common/cityhash.h" #include "common/common_types.h" @@ -33,4 +34,36 @@ static inline u64 ComputeStructHash64(const T& data) { return ComputeHash64(&data, sizeof(data)); } +/// A helper template that ensures the padding in a struct is initialized by memsetting to 0. +template +struct HashableStruct { + /* + * 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& o) const { + return std::memcmp(&state, &o.state, sizeof(T)) == 0; + }; + + bool operator!=(const HashableStruct& o) const { + return !(*this == o); + }; + + size_t Hash() const { + return Common::ComputeStructHash64(state); + } +}; + } // namespace Common diff --git a/src/video_core/renderer_opengl/gl_shader_gen.cpp b/src/video_core/renderer_opengl/gl_shader_gen.cpp index a9f7ecbfe7..d17597aa2e 100644 --- a/src/video_core/renderer_opengl/gl_shader_gen.cpp +++ b/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; diff --git a/src/video_core/renderer_opengl/gl_shader_gen.h b/src/video_core/renderer_opengl/gl_shader_gen.h index c67ceb54dc..7d14d2c710 100644 --- a/src/video_core/renderer_opengl/gl_shader_gen.h +++ b/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 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 { /// 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 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 { size_t operator()(const GLShader::PicaShaderConfig& k) const { - return Common::ComputeStructHash64(k.state); + return k.Hash(); } }; } // namespace std From 9c785814e7ce54652534cdad45c2bbecc6fa1a84 Mon Sep 17 00:00:00 2001 From: wwylele Date: Wed, 14 Mar 2018 15:37:47 +0200 Subject: [PATCH 2/2] Common/Hash: static_assert on the type passed to HashableStruct --- src/common/hash.h | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/src/common/hash.h b/src/common/hash.h index 042f4e5169..73c326980e 100644 --- a/src/common/hash.h +++ b/src/common/hash.h @@ -22,21 +22,23 @@ 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 - * standard layout), it is also critical that either the struct includes no padding, or that any - * padding is initialized to a known value by memsetting the struct to 0 before filling it in. + * Computes a 64-bit hash of a struct. In addition to being trivially copyable, it is also critical + * that either the struct includes no padding, or that any padding is initialized to a known value + * by memsetting the struct to 0 before filling it in. */ template static inline u64 ComputeStructHash64(const T& data) { - static_assert( - std::is_trivially_copyable::value && std::is_standard_layout::value, - "Type passed to ComputeStructHash64 must be trivially copyable and standard layout"); + static_assert(std::is_trivially_copyable(), + "Type passed to ComputeStructHash64 must be trivially copyable"); return ComputeHash64(&data, sizeof(data)); } /// A helper template that ensures the padding in a struct is initialized by memsetting to 0. template 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(), "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