From 149a97e396fd1ac22df8ad9d47d4a23e3ef46d25 Mon Sep 17 00:00:00 2001 From: Lioncash Date: Thu, 30 May 2019 01:05:06 -0400 Subject: [PATCH] VideoCommon: Remove unnecessary memset on ShaderUid instances. Zero-initialization zeroes out all members and padding bits, so this is safe to do. While we're at it, also add static assertions that enforce the necessary requirements of a UID type explicitly within the ShaderUid class. This way, we can remove several memset calls around the shader generation code that makes sure the underlying UID data is zeroed out. Now our ShaderUid class enforces this for us, so we don't need to care about it at the usage sites. --- Source/Core/VideoCommon/GeometryShaderGen.cpp | 9 ++---- Source/Core/VideoCommon/GeometryShaderGen.h | 4 +-- Source/Core/VideoCommon/PixelShaderGen.cpp | 6 ++-- Source/Core/VideoCommon/PixelShaderGen.h | 2 +- Source/Core/VideoCommon/ShaderGenCommon.h | 30 ++++++++----------- .../VideoCommon/TextureConverterShaderGen.cpp | 7 ++--- Source/Core/VideoCommon/UberShaderPixel.cpp | 10 +++---- Source/Core/VideoCommon/UberShaderPixel.h | 2 +- Source/Core/VideoCommon/UberShaderVertex.cpp | 8 ++--- Source/Core/VideoCommon/UberShaderVertex.h | 2 +- Source/Core/VideoCommon/VertexShaderGen.cpp | 9 ++---- Source/Core/VideoCommon/VertexShaderGen.h | 2 +- 12 files changed, 37 insertions(+), 54 deletions(-) diff --git a/Source/Core/VideoCommon/GeometryShaderGen.cpp b/Source/Core/VideoCommon/GeometryShaderGen.cpp index e6fa3d3913..90446ca40d 100644 --- a/Source/Core/VideoCommon/GeometryShaderGen.cpp +++ b/Source/Core/VideoCommon/GeometryShaderGen.cpp @@ -5,7 +5,6 @@ #include "VideoCommon/GeometryShaderGen.h" #include -#include #include "Common/CommonTypes.h" #include "VideoCommon/DriverDetails.h" @@ -27,10 +26,9 @@ bool geometry_shader_uid_data::IsPassthrough() const GeometryShaderUid GetGeometryShaderUid(PrimitiveType primitive_type) { - ShaderUid out; - geometry_shader_uid_data* uid_data = out.GetUidData(); - memset(uid_data, 0, sizeof(geometry_shader_uid_data)); + GeometryShaderUid out; + geometry_shader_uid_data* const uid_data = out.GetUidData(); uid_data->primitive_type = static_cast(primitive_type); uid_data->numTexGens = xfmem.numTexGen.numTexGens; @@ -364,7 +362,6 @@ static void EndPrimitive(ShaderCode& out, const ShaderHostConfig& host_config, void EnumerateGeometryShaderUids(const std::function& callback) { GeometryShaderUid uid; - std::memset(&uid, 0, sizeof(uid)); const std::array primitive_lut = { {g_ActiveConfig.backend_info.bSupportsPrimitiveRestart ? PrimitiveType::TriangleStrip : @@ -372,7 +369,7 @@ void EnumerateGeometryShaderUids(const std::function(); + geometry_shader_uid_data* const guid = uid.GetUidData(); guid->primitive_type = static_cast(primitive); for (u32 texgens = 0; texgens <= 8; texgens++) diff --git a/Source/Core/VideoCommon/GeometryShaderGen.h b/Source/Core/VideoCommon/GeometryShaderGen.h index 0c688d30a7..ba73e3875f 100644 --- a/Source/Core/VideoCommon/GeometryShaderGen.h +++ b/Source/Core/VideoCommon/GeometryShaderGen.h @@ -12,7 +12,6 @@ enum class APIType; #pragma pack(1) - struct geometry_shader_uid_data { u32 NumValues() const { return sizeof(geometry_shader_uid_data); } @@ -21,10 +20,9 @@ struct geometry_shader_uid_data u32 numTexGens : 4; u32 primitive_type : 2; }; - #pragma pack() -typedef ShaderUid GeometryShaderUid; +using GeometryShaderUid = ShaderUid; ShaderCode GenerateGeometryShaderCode(APIType ApiType, const ShaderHostConfig& host_config, const geometry_shader_uid_data* uid_data); diff --git a/Source/Core/VideoCommon/PixelShaderGen.cpp b/Source/Core/VideoCommon/PixelShaderGen.cpp index 314eab9995..72402ab4c8 100644 --- a/Source/Core/VideoCommon/PixelShaderGen.cpp +++ b/Source/Core/VideoCommon/PixelShaderGen.cpp @@ -6,7 +6,6 @@ #include #include -#include #include "Common/Assert.h" #include "Common/CommonTypes.h" @@ -162,9 +161,8 @@ static const char* tevAOutputTable[] = {"prev.a", "c0.a", "c1.a", "c2.a"}; PixelShaderUid GetPixelShaderUid() { PixelShaderUid out; - pixel_shader_uid_data* uid_data = out.GetUidData(); - memset(uid_data, 0, sizeof(*uid_data)); + pixel_shader_uid_data* const uid_data = out.GetUidData(); uid_data->useDstAlpha = bpmem.dstalpha.enable && bpmem.blendmode.alphaupdate && bpmem.zcontrol.pixel_format == PEControl::RGBA6_Z24; @@ -340,7 +338,7 @@ PixelShaderUid GetPixelShaderUid() void ClearUnusedPixelShaderUidBits(APIType ApiType, const ShaderHostConfig& host_config, PixelShaderUid* uid) { - pixel_shader_uid_data* uid_data = uid->GetUidData(); + pixel_shader_uid_data* const uid_data = uid->GetUidData(); // OpenGL and Vulkan convert implicitly normalized color outputs to their uint representation. // Therefore, it is not necessary to use a uint output on these backends. We also disable the diff --git a/Source/Core/VideoCommon/PixelShaderGen.h b/Source/Core/VideoCommon/PixelShaderGen.h index 91c19300ba..234ff875ca 100644 --- a/Source/Core/VideoCommon/PixelShaderGen.h +++ b/Source/Core/VideoCommon/PixelShaderGen.h @@ -162,7 +162,7 @@ struct pixel_shader_uid_data }; #pragma pack() -typedef ShaderUid PixelShaderUid; +using PixelShaderUid = ShaderUid; ShaderCode GeneratePixelShaderCode(APIType ApiType, const ShaderHostConfig& host_config, const pixel_shader_uid_data* uid_data); diff --git a/Source/Core/VideoCommon/ShaderGenCommon.h b/Source/Core/VideoCommon/ShaderGenCommon.h index 216f791df5..3dd125b2ca 100644 --- a/Source/Core/VideoCommon/ShaderGenCommon.h +++ b/Source/Core/VideoCommon/ShaderGenCommon.h @@ -8,6 +8,7 @@ #include #include #include +#include #include #include "Common/CommonTypes.h" @@ -48,17 +49,6 @@ public: * Tells us that a specific constant range (including last_index) is being used by the shader */ void SetConstantsUsed(unsigned int first_index, unsigned int last_index) {} - /* - * Returns a pointer to an internally stored object of the uid_data type. - * @warning since most child classes use the default implementation you shouldn't access this - * directly without adding precautions against nullptr access (e.g. via adding a dummy structure, - * cf. the vertex/pixel shader generators) - */ - template - uid_data* GetUidData() - { - return nullptr; - } }; /* @@ -74,6 +64,9 @@ template class ShaderUid : public ShaderGeneratorInterface { public: + static_assert(std::is_trivially_copyable_v, + "uid_data must be a trivially copyable type"); + bool operator==(const ShaderUid& obj) const { return memcmp(this->values, obj.values, data.NumValues() * sizeof(*values)) == 0; @@ -90,19 +83,22 @@ public: return memcmp(this->values, obj.values, data.NumValues() * sizeof(*values)) < 0; } - template - uid_data2* GetUidData() - { - return &data; - } + // Returns a pointer to an internally stored object of the uid_data type. + uid_data* GetUidData() { return &data; } + + // Returns a pointer to an internally stored object of the uid_data type. const uid_data* GetUidData() const { return &data; } + + // Returns the raw bytes that make up the shader UID. const u8* GetUidDataRaw() const { return &values[0]; } + + // Returns the size of the underlying UID data structure in bytes. size_t GetUidDataSize() const { return sizeof(values); } private: union { - uid_data data; + uid_data data{}; u8 values[sizeof(uid_data)]; }; }; diff --git a/Source/Core/VideoCommon/TextureConverterShaderGen.cpp b/Source/Core/VideoCommon/TextureConverterShaderGen.cpp index df46254d91..9bbb4e2892 100644 --- a/Source/Core/VideoCommon/TextureConverterShaderGen.cpp +++ b/Source/Core/VideoCommon/TextureConverterShaderGen.cpp @@ -2,13 +2,11 @@ // Licensed under GPLv2+ // Refer to the license.txt file included. -#include -#include +#include "VideoCommon/TextureConverterShaderGen.h" #include "Common/Assert.h" #include "Common/CommonTypes.h" #include "VideoCommon/BPMemory.h" -#include "VideoCommon/TextureConverterShaderGen.h" #include "VideoCommon/VideoCommon.h" #include "VideoCommon/VideoConfig.h" @@ -18,9 +16,8 @@ TCShaderUid GetShaderUid(EFBCopyFormat dst_format, bool is_depth_copy, bool is_i bool scale_by_half, bool copy_filter) { TCShaderUid out; - UidData* uid_data = out.GetUidData(); - memset(uid_data, 0, sizeof(*uid_data)); + UidData* const uid_data = out.GetUidData(); uid_data->dst_format = dst_format; uid_data->efb_has_alpha = bpmem.zcontrol.pixel_format == PEControl::RGBA6_Z24; uid_data->is_depth_copy = is_depth_copy; diff --git a/Source/Core/VideoCommon/UberShaderPixel.cpp b/Source/Core/VideoCommon/UberShaderPixel.cpp index 097b6456bd..6d2886b256 100644 --- a/Source/Core/VideoCommon/UberShaderPixel.cpp +++ b/Source/Core/VideoCommon/UberShaderPixel.cpp @@ -14,8 +14,8 @@ namespace UberShader PixelShaderUid GetPixelShaderUid() { PixelShaderUid out; - pixel_ubershader_uid_data* uid_data = out.GetUidData(); - memset(uid_data, 0, sizeof(*uid_data)); + + pixel_ubershader_uid_data* const uid_data = out.GetUidData(); uid_data->num_texgens = xfmem.numTexGen.numTexGens; uid_data->early_depth = bpmem.UseEarlyDepthTest() && @@ -26,13 +26,14 @@ PixelShaderUid GetPixelShaderUid() (!g_ActiveConfig.bFastDepthCalc && bpmem.zmode.testenable && !uid_data->early_depth) || (bpmem.zmode.testenable && bpmem.genMode.zfreeze); uid_data->uint_output = bpmem.blendmode.UseLogicOp(); + return out; } void ClearUnusedPixelShaderUidBits(APIType ApiType, const ShaderHostConfig& host_config, PixelShaderUid* uid) { - pixel_ubershader_uid_data* uid_data = uid->GetUidData(); + pixel_ubershader_uid_data* const uid_data = uid->GetUidData(); // OpenGL and Vulkan convert implicitly normalized color outputs to their uint representation. // Therefore, it is not necessary to use a uint output on these backends. We also disable the @@ -1390,11 +1391,10 @@ ShaderCode GenPixelShader(APIType ApiType, const ShaderHostConfig& host_config, void EnumeratePixelShaderUids(const std::function& callback) { PixelShaderUid uid; - std::memset(&uid, 0, sizeof(uid)); for (u32 texgens = 0; texgens <= 8; texgens++) { - auto* puid = uid.GetUidData(); + pixel_ubershader_uid_data* const puid = uid.GetUidData(); puid->num_texgens = texgens; for (u32 early_depth = 0; early_depth < 2; early_depth++) diff --git a/Source/Core/VideoCommon/UberShaderPixel.h b/Source/Core/VideoCommon/UberShaderPixel.h index 7b96d7952f..3575526ed2 100644 --- a/Source/Core/VideoCommon/UberShaderPixel.h +++ b/Source/Core/VideoCommon/UberShaderPixel.h @@ -21,7 +21,7 @@ struct pixel_ubershader_uid_data }; #pragma pack() -typedef ShaderUid PixelShaderUid; +using PixelShaderUid = ShaderUid; PixelShaderUid GetPixelShaderUid(); diff --git a/Source/Core/VideoCommon/UberShaderVertex.cpp b/Source/Core/VideoCommon/UberShaderVertex.cpp index 4032e0df40..5fb30aaf03 100644 --- a/Source/Core/VideoCommon/UberShaderVertex.cpp +++ b/Source/Core/VideoCommon/UberShaderVertex.cpp @@ -15,9 +15,10 @@ namespace UberShader VertexShaderUid GetVertexShaderUid() { VertexShaderUid out; - vertex_ubershader_uid_data* uid_data = out.GetUidData(); - memset(uid_data, 0, sizeof(*uid_data)); + + vertex_ubershader_uid_data* const uid_data = out.GetUidData(); uid_data->num_texgens = xfmem.numTexGen.numTexGens; + return out; } @@ -487,11 +488,10 @@ void GenVertexShaderTexGens(APIType ApiType, u32 numTexgen, ShaderCode& out) void EnumerateVertexShaderUids(const std::function& callback) { VertexShaderUid uid; - std::memset(&uid, 0, sizeof(uid)); for (u32 texgens = 0; texgens <= 8; texgens++) { - auto* vuid = uid.GetUidData(); + vertex_ubershader_uid_data* const vuid = uid.GetUidData(); vuid->num_texgens = texgens; callback(uid); } diff --git a/Source/Core/VideoCommon/UberShaderVertex.h b/Source/Core/VideoCommon/UberShaderVertex.h index cbbcb1459e..a66c28c29c 100644 --- a/Source/Core/VideoCommon/UberShaderVertex.h +++ b/Source/Core/VideoCommon/UberShaderVertex.h @@ -18,7 +18,7 @@ struct vertex_ubershader_uid_data }; #pragma pack() -typedef ShaderUid VertexShaderUid; +using VertexShaderUid = ShaderUid; VertexShaderUid GetVertexShaderUid(); diff --git a/Source/Core/VideoCommon/VertexShaderGen.cpp b/Source/Core/VideoCommon/VertexShaderGen.cpp index 678df11a0e..bcd5f79bae 100644 --- a/Source/Core/VideoCommon/VertexShaderGen.cpp +++ b/Source/Core/VideoCommon/VertexShaderGen.cpp @@ -2,7 +2,7 @@ // Licensed under GPLv2+ // Refer to the license.txt file included. -#include +#include "VideoCommon/VertexShaderGen.h" #include "Common/Assert.h" #include "Common/CommonTypes.h" @@ -10,20 +10,17 @@ #include "VideoCommon/LightingShaderGen.h" #include "VideoCommon/NativeVertexFormat.h" #include "VideoCommon/VertexLoaderManager.h" -#include "VideoCommon/VertexShaderGen.h" #include "VideoCommon/VideoCommon.h" #include "VideoCommon/VideoConfig.h" #include "VideoCommon/XFMemory.h" VertexShaderUid GetVertexShaderUid() { - VertexShaderUid out; - vertex_shader_uid_data* uid_data = out.GetUidData(); - memset(uid_data, 0, sizeof(*uid_data)); - ASSERT(bpmem.genMode.numtexgens == xfmem.numTexGen.numTexGens); ASSERT(bpmem.genMode.numcolchans == xfmem.numChan.numColorChans); + VertexShaderUid out; + vertex_shader_uid_data* const uid_data = out.GetUidData(); uid_data->numTexGens = xfmem.numTexGen.numTexGens; uid_data->components = VertexLoaderManager::g_current_components; uid_data->numColorChans = xfmem.numChan.numColorChans; diff --git a/Source/Core/VideoCommon/VertexShaderGen.h b/Source/Core/VideoCommon/VertexShaderGen.h index 7d533c4061..cfb47b35c6 100644 --- a/Source/Core/VideoCommon/VertexShaderGen.h +++ b/Source/Core/VideoCommon/VertexShaderGen.h @@ -65,7 +65,7 @@ struct vertex_shader_uid_data }; #pragma pack() -typedef ShaderUid VertexShaderUid; +using VertexShaderUid = ShaderUid; VertexShaderUid GetVertexShaderUid(); ShaderCode GenerateVertexShaderCode(APIType api_type, const ShaderHostConfig& host_config,