From 617f9d953259a113d1c00c588c8c70837a791ae3 Mon Sep 17 00:00:00 2001 From: Stenzek Date: Sat, 2 Jan 2016 16:20:01 +1000 Subject: [PATCH] ShaderGen: Remove virtual methods from ShaderGeneratorInterface, move string buffer to ShaderCode This fixes the crashes occuring at startup with a non-empty shader cache. Because LinearDiskCache reads/writes to the storage of ShaderUid, ShaderUid must be trivially copyable. Additionally, adds a static assert to LinearDiskCache to ensure this doesn't happen in the future. The initialization of ShaderUid data has been moved to the code generation functions, so the above condition holds true. --- Source/Core/Common/LinearDiskCache.h | 10 +++++ .../VideoBackends/OGL/ProgramShaderCache.h | 4 -- Source/Core/VideoCommon/GeometryShaderGen.cpp | 4 +- Source/Core/VideoCommon/PixelShaderGen.cpp | 4 +- Source/Core/VideoCommon/ShaderGenCommon.h | 41 ++++++++----------- Source/Core/VideoCommon/VertexShaderGen.cpp | 4 +- 6 files changed, 35 insertions(+), 32 deletions(-) diff --git a/Source/Core/Common/LinearDiskCache.h b/Source/Core/Common/LinearDiskCache.h index b9dd2b61d4..e37f93d1e1 100644 --- a/Source/Core/Common/LinearDiskCache.h +++ b/Source/Core/Common/LinearDiskCache.h @@ -7,6 +7,7 @@ #include #include #include +#include #include "Common/CommonTypes.h" #include "Common/FileUtil.h" @@ -53,6 +54,15 @@ public: { using std::ios_base; + // Since we're reading/writing directly to the storage of K instances, + // K must be trivially copyable. TODO: Remove #if once GCC 5.0 is a + // minimum requirement. +#if defined(__GNUC__) && !defined(__clang__) && __GNUC__ < 5 + static_assert(std::has_trivial_copy_constructor::value, "K must be a trivially copyable type"); +#else + static_assert(std::is_trivially_copyable::value, "K must be a trivially copyable type"); +#endif + // close any currently opened file Close(); m_num_entries = 0; diff --git a/Source/Core/VideoBackends/OGL/ProgramShaderCache.h b/Source/Core/VideoBackends/OGL/ProgramShaderCache.h index d20da70d2f..17fe2494bc 100644 --- a/Source/Core/VideoBackends/OGL/ProgramShaderCache.h +++ b/Source/Core/VideoBackends/OGL/ProgramShaderCache.h @@ -23,10 +23,6 @@ public: PixelShaderUid puid; GeometryShaderUid guid; - SHADERUID() {} - - SHADERUID(const SHADERUID& r) : vuid(r.vuid), puid(r.puid), guid(r.guid) {} - bool operator <(const SHADERUID& r) const { if (puid < r.puid) diff --git a/Source/Core/VideoCommon/GeometryShaderGen.cpp b/Source/Core/VideoCommon/GeometryShaderGen.cpp index 9c1ccd3071..602adb7e21 100644 --- a/Source/Core/VideoCommon/GeometryShaderGen.cpp +++ b/Source/Core/VideoCommon/GeometryShaderGen.cpp @@ -34,7 +34,9 @@ static T GenerateGeometryShader(u32 primitive_type, API_TYPE ApiType) // Non-uid template parameters will write to the dummy data (=> gets optimized out) geometry_shader_uid_data dummy_data; geometry_shader_uid_data* uid_data = out.template GetUidData(); - if (uid_data == nullptr) + if (uid_data != nullptr) + memset(uid_data, 0, sizeof(*uid_data)); + else uid_data = &dummy_data; uid_data->primitive_type = primitive_type; diff --git a/Source/Core/VideoCommon/PixelShaderGen.cpp b/Source/Core/VideoCommon/PixelShaderGen.cpp index 957c0b0fe7..04c0df00c6 100644 --- a/Source/Core/VideoCommon/PixelShaderGen.cpp +++ b/Source/Core/VideoCommon/PixelShaderGen.cpp @@ -168,7 +168,9 @@ static T GeneratePixelShader(DSTALPHA_MODE dstAlphaMode, API_TYPE ApiType) // Non-uid template parameters will write to the dummy data (=> gets optimized out) pixel_shader_uid_data dummy_data; pixel_shader_uid_data* uid_data = out.template GetUidData(); - if (uid_data == nullptr) + if (uid_data != nullptr) + memset(uid_data, 0, sizeof(*uid_data)); + else uid_data = &dummy_data; unsigned int numStages = bpmem.genMode.numtevstages + 1; diff --git a/Source/Core/VideoCommon/ShaderGenCommon.h b/Source/Core/VideoCommon/ShaderGenCommon.h index e8ceafe239..acda202d94 100644 --- a/Source/Core/VideoCommon/ShaderGenCommon.h +++ b/Source/Core/VideoCommon/ShaderGenCommon.h @@ -18,30 +18,23 @@ #include "VideoCommon/XFMemory.h" /** - * Common interface for classes that need to go through the shader generation path (GenerateVertexShader, GeneratePixelShader) + * Common interface for classes that need to go through the shader generation path (GenerateVertexShader, GenerateGeometryShader, GeneratePixelShader) * In particular, this includes the shader code generator (ShaderCode). * A different class (ShaderUid) can be used to uniquely identify each ShaderCode object. * More interesting things can be done with this, e.g. ShaderConstantProfile checks what shader constants are being used. This can be used to optimize buffer management. - * Each of the ShaderCode, ShaderUid and ShaderConstantProfile child classes only implement the subset of ShaderGeneratorInterface methods that are required for the specific tasks. + * If the class does not use one or more of these methods (e.g. Uid class does not need code), the method will be defined as a no-op by the base class, and the call + * should be optimized out. The reason for this implementation is so that shader selection/generation can be done in two passes, with only a cache lookup being + * required if the shader has already been generated. */ class ShaderGeneratorInterface { public: - virtual ~ShaderGeneratorInterface() - { - } - - /* - * Returns a read pointer to the internal buffer. - */ - const std::string& GetBuffer() const { return m_buffer; } - /* * Used when the shader generator would write a piece of ShaderCode. * Can be used like printf. * @note In the ShaderCode implementation, this does indeed write the parameter string to an internal buffer. However, you're free to do whatever you like with the parameter. */ - virtual void Write(const char*, ...) + void Write(const char*, ...) #ifdef __GNUC__ __attribute__((format(printf, 2, 3))) #endif @@ -51,7 +44,7 @@ public: /* * Tells us that a specific constant range (including last_index) is being used by the shader */ - virtual void SetConstantsUsed(unsigned int first_index, unsigned int last_index) {} + void SetConstantsUsed(unsigned int first_index, unsigned int last_index) {} /* * Returns a pointer to an internally stored object of the uid_data type. @@ -59,27 +52,19 @@ public: */ template uid_data* GetUidData() { return nullptr; } - -protected: - std::string m_buffer; }; -/** +/* * Shader UID class used to uniquely identify the ShaderCode output written in the shader generator. * uid_data can be any struct of parameters that uniquely identify each shader code output. * Unless performance is not an issue, uid_data should be tightly packed to reduce memory footprint. * Shader generators will write to specific uid_data fields; ShaderUid methods will only read raw u32 values from a union. + * NOTE: Because LinearDiskCache reads and writes the storage associated with a ShaderUid instance, ShaderUid must be trivially copyable. */ template class ShaderUid : public ShaderGeneratorInterface { public: - ShaderUid() - { - // TODO: Move to Shadergen => can be optimized out - memset(values, 0, sizeof(values)); - } - bool operator == (const ShaderUid& obj) const { return memcmp(this->values, obj.values, data.NumValues() * sizeof(*values)) == 0; @@ -119,7 +104,9 @@ public: m_buffer.reserve(16384); } - void Write(const char* fmt, ...) override + const std::string& GetBuffer() const { return m_buffer; } + + void Write(const char* fmt, ...) #ifdef __GNUC__ __attribute__((format(printf, 2, 3))) #endif @@ -129,6 +116,9 @@ public: m_buffer += StringFromFormatV(fmt, arglist); va_end(arglist); } + +protected: + std::string m_buffer; }; /** @@ -139,7 +129,7 @@ class ShaderConstantProfile : public ShaderGeneratorInterface public: ShaderConstantProfile(int num_constants) { constant_usage.resize(num_constants); } - void SetConstantsUsed(unsigned int first_index, unsigned int last_index) override + void SetConstantsUsed(unsigned int first_index, unsigned int last_index) { for (unsigned int i = first_index; i < last_index + 1; ++i) constant_usage[i] = true; @@ -151,6 +141,7 @@ public: return true; //return constant_usage[index]; } + private: std::vector constant_usage; // TODO: Is vector appropriate here? }; diff --git a/Source/Core/VideoCommon/VertexShaderGen.cpp b/Source/Core/VideoCommon/VertexShaderGen.cpp index 3a9a167a64..a9007978fc 100644 --- a/Source/Core/VideoCommon/VertexShaderGen.cpp +++ b/Source/Core/VideoCommon/VertexShaderGen.cpp @@ -19,7 +19,9 @@ static T GenerateVertexShader(API_TYPE api_type) // Non-uid template parameters will write to the dummy data (=> gets optimized out) vertex_shader_uid_data dummy_data; vertex_shader_uid_data* uid_data = out.template GetUidData(); - if (uid_data == nullptr) + if (uid_data != nullptr) + memset(uid_data, 0, sizeof(*uid_data)); + else uid_data = &dummy_data; _assert_(bpmem.genMode.numtexgens == xfmem.numTexGen.numTexGens);