From ea36e7a239b4162c49c25e8c19703f1cd4cfdf8d Mon Sep 17 00:00:00 2001 From: Dale Glass Date: Wed, 8 Jan 2020 00:12:30 +0100 Subject: [PATCH 1/4] Fix thousands of 'is too small to hold all values' warnings on Linux MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When building with GCC, it generates 3020 warnings like this: libraries/gpu/src/gpu/State.h:208:18: warning: ‘gpu::State::BlendFunction::destColor’ is too small to hold all values of ‘enum gpu::State::BlendArg’ This is because ‘enum gpu::State::BlendArg’ is declared as an enum of uint16 size, and gpu::State::BlendFunction::destColor is a bit field. GCC correctly deduces that a 16 bit value won't always fit in a 4 bit field, and emits a warning. The problem is that the amount is such that it floods the output and hides other warnings. Changing the 'enum foo : uint16' declarations to 'enum foo' stops gcc from emitting the warning. Making this change required the removal of a set of assertion checks. At least empirically they seem unnecessary -- the interface compiles and runs fine. --- libraries/gpu/src/gpu/Format.h | 2 +- libraries/gpu/src/gpu/State.h | 28 ++++++---------------------- 2 files changed, 7 insertions(+), 23 deletions(-) diff --git a/libraries/gpu/src/gpu/Format.h b/libraries/gpu/src/gpu/Format.h index 25bb2baba4..61518edb6c 100644 --- a/libraries/gpu/src/gpu/Format.h +++ b/libraries/gpu/src/gpu/Format.h @@ -377,7 +377,7 @@ public: }; -enum ComparisonFunction : uint16 { +enum ComparisonFunction { NEVER = 0, LESS, EQUAL, diff --git a/libraries/gpu/src/gpu/State.h b/libraries/gpu/src/gpu/State.h index 2e8a3f2cab..7467d86ac6 100755 --- a/libraries/gpu/src/gpu/State.h +++ b/libraries/gpu/src/gpu/State.h @@ -46,7 +46,7 @@ public: typedef ::gpu::ComparisonFunction ComparisonFunction; - enum FillMode : uint8 + enum FillMode { FILL_POINT = 0, FILL_LINE, @@ -55,7 +55,7 @@ public: NUM_FILL_MODES, }; - enum CullMode : uint8 + enum CullMode { CULL_NONE = 0, CULL_FRONT, @@ -64,7 +64,7 @@ public: NUM_CULL_MODES, }; - enum StencilOp : uint16 + enum StencilOp { STENCIL_OP_KEEP = 0, STENCIL_OP_ZERO, @@ -78,7 +78,7 @@ public: NUM_STENCIL_OPS, }; - enum BlendArg : uint16 + enum BlendArg { ZERO = 0, ONE, @@ -99,7 +99,7 @@ public: NUM_BLEND_ARGS, }; - enum BlendOp : uint16 + enum BlendOp { BLEND_OP_ADD = 0, BLEND_OP_SUBTRACT, @@ -110,7 +110,7 @@ public: NUM_BLEND_OPS, }; - enum ColorMask : uint8 + enum ColorMask { WRITE_NONE = 0, WRITE_RED = 1, @@ -140,8 +140,6 @@ public: bool operator!=(const DepthTest& right) const { return getRaw() != right.getRaw(); } }; - static_assert(sizeof(DepthTest) == sizeof(uint32_t), "DepthTest size check"); - struct StencilTest { ComparisonFunction function : 4; StencilOp failOp : 4; @@ -282,20 +280,6 @@ public: Flags flags; }; - static_assert(offsetof(Data, depthBias) == 0, "Data offsets"); - static_assert(offsetof(Data, depthBiasSlopeScale) == 4, "Data offsets"); - static_assert(offsetof(Data, depthTest) == 8, "Data offsets"); - static_assert(offsetof(Data, stencilActivation) == 12, "Data offsets"); - static_assert(offsetof(Data, stencilTestFront) == 16, "Data offsets"); - static_assert(offsetof(Data, stencilTestBack) == 20, "Data offsets"); - static_assert(offsetof(Data, sampleMask) == 24, "Data offsets"); - static_assert(offsetof(Data, blendFunction) == 28, "Data offsets"); - static_assert(offsetof(Data, fillMode) == 32, "Data offsets"); - static_assert(offsetof(Data, cullMode) == 33, "Data offsets"); - static_assert(offsetof(Data, colorWriteMask) == 34, "Data offsets"); - static_assert(offsetof(Data, flags) == 35, "Data offsets"); - static_assert(sizeof(Data) == 36, "Data Size Check"); - std::string getKey() const; // The unique default values for all the fields From 8f65a904539f8d42f991d85c1a62936c577e3e09 Mon Sep 17 00:00:00 2001 From: Dale Glass Date: Tue, 14 Jan 2020 23:00:08 +0100 Subject: [PATCH 2/4] Remove getRaw() from DepthTest State::getKey removed because it breaks and is not referenced from anywhere. --- .../src/gpu/gl/GLBackendState.cpp | 5 +---- .../gpu-gl-common/src/gpu/gl/GLState.cpp | 2 +- libraries/gpu/src/gpu/State.cpp | 17 ---------------- libraries/gpu/src/gpu/State.h | 20 +++++++++++++------ 4 files changed, 16 insertions(+), 28 deletions(-) diff --git a/libraries/gpu-gl-common/src/gpu/gl/GLBackendState.cpp b/libraries/gpu-gl-common/src/gpu/gl/GLBackendState.cpp index 5da32f39df..0c2c0ae744 100644 --- a/libraries/gpu-gl-common/src/gpu/gl/GLBackendState.cpp +++ b/libraries/gpu-gl-common/src/gpu/gl/GLBackendState.cpp @@ -204,10 +204,7 @@ void GLBackend::do_setStateDepthTest(State::DepthTest test) { glDepthFunc(COMPARISON_TO_GL[test.getFunction()]); } if (CHECK_GL_ERROR()) { - qCDebug(gpulogging) << "DepthTest" << (test.isEnabled() ? "Enabled" : "Disabled") - << "Mask=" << (test.getWriteMask() ? "Write" : "no Write") - << "Func=" << (uint16_t)test.getFunction() - << "Raw=" << test.getRaw(); + qCDebug(gpulogging) << "DepthTest = " << test; } _pipeline._stateCache.depthTest = test; } diff --git a/libraries/gpu-gl-common/src/gpu/gl/GLState.cpp b/libraries/gpu-gl-common/src/gpu/gl/GLState.cpp index 89ba80031b..3236fa05e7 100644 --- a/libraries/gpu-gl-common/src/gpu/gl/GLState.cpp +++ b/libraries/gpu-gl-common/src/gpu/gl/GLState.cpp @@ -119,7 +119,7 @@ void generateDepthBias(GLState::Commands& commands, const State& state) { } void generateDepthTest(GLState::Commands& commands, const State::DepthTest& test) { - commands.push_back(std::make_shared(&GLBackend::do_setStateDepthTest, int32(test.getRaw()))); + commands.push_back(std::make_shared(&GLBackend::do_setStateDepthTest, test)); } void generateStencil(GLState::Commands& commands, const State& state) { diff --git a/libraries/gpu/src/gpu/State.cpp b/libraries/gpu/src/gpu/State.cpp index fab0438f75..81a43f87ae 100755 --- a/libraries/gpu/src/gpu/State.cpp +++ b/libraries/gpu/src/gpu/State.cpp @@ -94,20 +94,3 @@ static std::string hex(T t) { stream << std::hex << t; return stream.str(); } - -std::string State::getKey() const { - std::string key; - key = hex(*(int*)&_values.depthBias); - key += ":" + hex(*(int*)&_values.depthBiasSlopeScale); - key += ":" + hex(_values.depthTest.getRaw()); - key += ":" + hex(_values.stencilActivation.getRaw()); - key += ":" + hex(_values.stencilTestFront.getRaw()); - key += ":" + hex(_values.stencilTestBack.getRaw()); - key += ":" + hex(_values.blendFunction.getRaw()); - key += ":" + hex(_values.sampleMask); - // fillMode, cullMode, colorMaskWrite and the flags consume 32 bits alltogether - static_assert(0 == offsetof(State::Data, fillMode) % 4, "Validate fillMode offset"); - key += ":" + hex(*(int*)&_values.fillMode); - return key; -} - diff --git a/libraries/gpu/src/gpu/State.h b/libraries/gpu/src/gpu/State.h index 7467d86ac6..30c3bbdc05 100755 --- a/libraries/gpu/src/gpu/State.h +++ b/libraries/gpu/src/gpu/State.h @@ -18,6 +18,7 @@ #include #include #include +#include // Why a macro and not a fancy template you will ask me ? // Because some of the fields are bool packed tightly in the State::Cache class @@ -134,10 +135,19 @@ public: ComparisonFunction getFunction() const { return function; } uint8 getWriteMask() const { return writeMask; } - int32 getRaw() const { return *(reinterpret_cast(this)); } - DepthTest(int32 raw) { *(reinterpret_cast(this)) = raw; } - bool operator==(const DepthTest& right) const { return getRaw() == right.getRaw(); } - bool operator!=(const DepthTest& right) const { return getRaw() != right.getRaw(); } + bool operator==(const DepthTest& right) const { + return writeMask == right.writeMask && enabled == right.enabled && function == right.function; + } + + bool operator!=(const DepthTest& right) const { + return writeMask != right.writeMask || enabled != right.enabled || function != right.function; + } + + operator QString() const { + return QString("{ writeMask = %1, enabled = %2, function = %3 }").arg(writeMask).arg(enabled).arg(function); + + } + }; struct StencilTest { @@ -280,8 +290,6 @@ public: Flags flags; }; - std::string getKey() const; - // The unique default values for all the fields static const Data DEFAULT; void setFillMode(FillMode fill) { SET_FIELD(FILL_MODE, fillMode, fill); } From 367c5f39dfc4e64f7ff159d863c7a19bb46a4295 Mon Sep 17 00:00:00 2001 From: Dale Glass Date: Thu, 16 Jan 2020 21:02:58 +0100 Subject: [PATCH 3/4] Finish removal of getRaw() and bit fields --- libraries/gpu/src/gpu/State.h | 127 +++++++++++++++++++++------------- 1 file changed, 78 insertions(+), 49 deletions(-) diff --git a/libraries/gpu/src/gpu/State.h b/libraries/gpu/src/gpu/State.h index 30c3bbdc05..ecb0043666 100755 --- a/libraries/gpu/src/gpu/State.h +++ b/libraries/gpu/src/gpu/State.h @@ -136,25 +136,26 @@ public: uint8 getWriteMask() const { return writeMask; } bool operator==(const DepthTest& right) const { - return writeMask == right.writeMask && enabled == right.enabled && function == right.function; + return + writeMask == right.writeMask && + enabled == right.enabled && + function == right.function; } bool operator!=(const DepthTest& right) const { - return writeMask != right.writeMask || enabled != right.enabled || function != right.function; + return !(right == *this); } operator QString() const { return QString("{ writeMask = %1, enabled = %2, function = %3 }").arg(writeMask).arg(enabled).arg(function); - } - }; struct StencilTest { - ComparisonFunction function : 4; - StencilOp failOp : 4; - StencilOp depthFailOp : 4; - StencilOp passOp : 4; + ComparisonFunction function; + StencilOp failOp; + StencilOp depthFailOp; + StencilOp passOp; int8 reference{ 0 }; uint8 readMask{ 0xff }; @@ -176,47 +177,56 @@ public: int8 getReference() const { return reference; } uint8 getReadMask() const { return readMask; } - int32 getRaw() const { return *(reinterpret_cast(this)); } - StencilTest(int32 raw) { *(reinterpret_cast(this)) = raw; } - bool operator==(const StencilTest& right) const { return getRaw() == right.getRaw(); } - bool operator!=(const StencilTest& right) const { return getRaw() != right.getRaw(); } + bool operator==(const StencilTest& right) const { + return + function == right.function && + failOp == right.failOp && + depthFailOp == right.depthFailOp && + passOp == right.passOp && + reference == right.reference && + readMask == right.readMask; + + } + + bool operator!=(const StencilTest &right) const { return !(right==*this); } }; - static_assert(sizeof(StencilTest) == sizeof(uint32_t), "StencilTest size check"); StencilTest stencilTestFront; struct StencilActivation { uint8 frontWriteMask = 0xFF; uint8 backWriteMask = 0xFF; - bool enabled : 1; - uint8 _spare1 : 7; - uint8 _spare2{ 0 }; + bool enabled; public: StencilActivation(bool enabled = false, uint8 frontWriteMask = 0xFF, uint8 backWriteMask = 0xFF) : - frontWriteMask(frontWriteMask), backWriteMask(backWriteMask), enabled(enabled), _spare1{ 0 } {} + frontWriteMask(frontWriteMask), backWriteMask(backWriteMask), enabled(enabled) {} bool isEnabled() const { return enabled; } uint8 getWriteMaskFront() const { return frontWriteMask; } uint8 getWriteMaskBack() const { return backWriteMask; } - int32 getRaw() const { return *(reinterpret_cast(this)); } - StencilActivation(int32 raw) { *(reinterpret_cast(this)) = raw; } - bool operator==(const StencilActivation& right) const { return getRaw() == right.getRaw(); } - bool operator!=(const StencilActivation& right) const { return getRaw() != right.getRaw(); } - }; + bool operator==(const StencilActivation& right) const { + return + frontWriteMask == right.frontWriteMask && + backWriteMask == right.backWriteMask && + enabled == right.enabled; + } - static_assert(sizeof(StencilActivation) == sizeof(uint32_t), "StencilActivation size check"); + bool operator!=(const StencilActivation& right) const { + return !(right == *this); + } + }; struct BlendFunction { // Using uint8 here will make the structure as a whole not align to 32 bits - uint16 enabled : 8; - BlendArg sourceColor : 4; - BlendArg sourceAlpha : 4; - BlendArg destColor : 4; - BlendArg destAlpha : 4; - BlendOp opColor : 4; - BlendOp opAlpha : 4; + uint16 enabled; + BlendArg sourceColor; + BlendArg sourceAlpha; + BlendArg destColor; + BlendArg destAlpha; + BlendOp opColor; + BlendOp opAlpha; public: BlendFunction(bool enabled, @@ -227,7 +237,7 @@ public: BlendOp operationAlpha, BlendArg destinationAlpha) : enabled(enabled), - sourceColor(sourceColor), sourceAlpha(sourceAlpha), + sourceColor(sourceColor), sourceAlpha(sourceAlpha), destColor(destinationColor), destAlpha(destinationAlpha), opColor(operationColor), opAlpha(operationAlpha) {} @@ -244,32 +254,51 @@ public: BlendArg getDestinationAlpha() const { return destAlpha; } BlendOp getOperationAlpha() const { return opAlpha; } - int32 getRaw() const { return *(reinterpret_cast(this)); } - BlendFunction(int32 raw) { *(reinterpret_cast(this)) = raw; } - bool operator==(const BlendFunction& right) const { return getRaw() == right.getRaw(); } - bool operator!=(const BlendFunction& right) const { return getRaw() != right.getRaw(); } - }; + bool operator==(const BlendFunction& right) const { + return + enabled == right.enabled && + sourceColor == right.sourceColor && + sourceAlpha == right.sourceAlpha && + destColor == right.destColor && + destAlpha == right.destAlpha && + opColor == right.opColor && + opAlpha == right.opAlpha; - static_assert(sizeof(BlendFunction) == sizeof(uint32_t), "BlendFunction size check"); + } + + bool operator!=(const BlendFunction& right) const { + return !(right == *this); + } + }; struct Flags { Flags() : frontFaceClockwise(false), depthClampEnable(false), scissorEnable(false), multisampleEnable(true), - antialisedLineEnable(true), alphaToCoverageEnable(false), _spare1(0) {} - bool frontFaceClockwise : 1; - bool depthClampEnable : 1; - bool scissorEnable : 1; - bool multisampleEnable : 1; - bool antialisedLineEnable : 1; - bool alphaToCoverageEnable : 1; - uint8 _spare1 : 2; + antialisedLineEnable(true), alphaToCoverageEnable(false) {} + bool frontFaceClockwise; + bool depthClampEnable; + bool scissorEnable; + bool multisampleEnable; + bool antialisedLineEnable; + bool alphaToCoverageEnable; - bool operator==(const Flags& right) const { return *(uint8*)this == *(uint8*)&right; } - bool operator!=(const Flags& right) const { return *(uint8*)this != *(uint8*)&right; } + + bool operator==(const Flags& right) const { + return + frontFaceClockwise == right.frontFaceClockwise && + depthClampEnable == right.depthClampEnable && + scissorEnable == right.scissorEnable && + multisampleEnable == right.multisampleEnable && + antialisedLineEnable == right.antialisedLineEnable && + alphaToCoverageEnable == right.alphaToCoverageEnable; + + } + + bool operator!=(const Flags& right) const { + return !(right == *this); + } }; - static_assert(sizeof(Flags) == sizeof(uint8), "Flags size check"); - // The Data class is the full explicit description of the State class fields value. // Useful for having one const static called Default for reference or for the gpu::Backend to keep track of the current value class Data { From bb51a000fb1e8a3217aa0e0b6d3163e8c965322a Mon Sep 17 00:00:00 2001 From: Dale Glass Date: Fri, 17 Jan 2020 00:41:16 +0100 Subject: [PATCH 4/4] Code style fixes --- libraries/gpu/src/gpu/State.h | 50 +++++++++++++++++------------------ 1 file changed, 25 insertions(+), 25 deletions(-) diff --git a/libraries/gpu/src/gpu/State.h b/libraries/gpu/src/gpu/State.h index ecb0043666..86bc5d4c5a 100755 --- a/libraries/gpu/src/gpu/State.h +++ b/libraries/gpu/src/gpu/State.h @@ -138,12 +138,12 @@ public: bool operator==(const DepthTest& right) const { return writeMask == right.writeMask && - enabled == right.enabled && - function == right.function; + enabled == right.enabled && + function == right.function; } bool operator!=(const DepthTest& right) const { - return !(right == *this); + return !(*this == right); } operator QString() const { @@ -179,12 +179,12 @@ public: bool operator==(const StencilTest& right) const { return - function == right.function && - failOp == right.failOp && + function == right.function && + failOp == right.failOp && depthFailOp == right.depthFailOp && - passOp == right.passOp && - reference == right.reference && - readMask == right.readMask; + passOp == right.passOp && + reference == right.reference && + readMask == right.readMask; } @@ -209,12 +209,12 @@ public: bool operator==(const StencilActivation& right) const { return frontWriteMask == right.frontWriteMask && - backWriteMask == right.backWriteMask && - enabled == right.enabled; + backWriteMask == right.backWriteMask && + enabled == right.enabled; } bool operator!=(const StencilActivation& right) const { - return !(right == *this); + return !(*this == right); } }; @@ -256,18 +256,18 @@ public: bool operator==(const BlendFunction& right) const { return - enabled == right.enabled && - sourceColor == right.sourceColor && - sourceAlpha == right.sourceAlpha && - destColor == right.destColor && - destAlpha == right.destAlpha && - opColor == right.opColor && - opAlpha == right.opAlpha; + enabled == right.enabled && + sourceColor == right.sourceColor && + sourceAlpha == right.sourceAlpha && + destColor == right.destColor && + destAlpha == right.destAlpha && + opColor == right.opColor && + opAlpha == right.opAlpha; } bool operator!=(const BlendFunction& right) const { - return !(right == *this); + return !(*this == right); } }; @@ -285,17 +285,17 @@ public: bool operator==(const Flags& right) const { return - frontFaceClockwise == right.frontFaceClockwise && - depthClampEnable == right.depthClampEnable && - scissorEnable == right.scissorEnable && - multisampleEnable == right.multisampleEnable && - antialisedLineEnable == right.antialisedLineEnable && + frontFaceClockwise == right.frontFaceClockwise && + depthClampEnable == right.depthClampEnable && + scissorEnable == right.scissorEnable && + multisampleEnable == right.multisampleEnable && + antialisedLineEnable == right.antialisedLineEnable && alphaToCoverageEnable == right.alphaToCoverageEnable; } bool operator!=(const Flags& right) const { - return !(right == *this); + return !(*this == right); } };