From 01384725c245a003e08233de00f259ee91a1b82f Mon Sep 17 00:00:00 2001 From: johnkslang Date: Fri, 14 Aug 2020 08:40:06 -0600 Subject: [PATCH] Fix #2366, fix #2358, correctly separate out numerical feature checking We need separate concepts for - total set of extensions ever enabled, for the back end - current state of extensions, for parsing - the set of features currently enabled for building the AST --- SPIRV/GlslangToSpv.cpp | 2 +- Test/baseResults/versionsErrors.frag.out | 2 - glslang/MachineIndependent/Intermediate.cpp | 62 ++++++++-------- glslang/MachineIndependent/ParseHelper.cpp | 2 + glslang/MachineIndependent/Versions.cpp | 37 +++++++++- glslang/MachineIndependent/intermOut.cpp | 2 +- .../MachineIndependent/localintermediate.h | 74 ++++++++++--------- glslang/MachineIndependent/parseVersions.h | 2 +- 8 files changed, 109 insertions(+), 74 deletions(-) diff --git a/SPIRV/GlslangToSpv.cpp b/SPIRV/GlslangToSpv.cpp index 5a39c1e3..c8554160 100644 --- a/SPIRV/GlslangToSpv.cpp +++ b/SPIRV/GlslangToSpv.cpp @@ -1444,7 +1444,7 @@ TGlslangToSpvTraverser::TGlslangToSpvTraverser(unsigned int spvVersion, // Add the source extensions const auto& sourceExtensions = glslangIntermediate->getRequestedExtensions(); for (auto it = sourceExtensions.begin(); it != sourceExtensions.end(); ++it) - builder.addSourceExtension(it->first.c_str()); + builder.addSourceExtension(it->c_str()); // Add the top-level modes for this shader. diff --git a/Test/baseResults/versionsErrors.frag.out b/Test/baseResults/versionsErrors.frag.out index b0726695..dbeb941a 100644 --- a/Test/baseResults/versionsErrors.frag.out +++ b/Test/baseResults/versionsErrors.frag.out @@ -7,7 +7,6 @@ ERROR: 4 compilation errors. No code generated. Shader version: 110 -Requested GL_ARB_texture_rectangle ERROR: node is still EOpNull! 0:42 Function Definition: main( ( global void) 0:42 Function Parameters: @@ -28,7 +27,6 @@ Linked fragment stage: Shader version: 110 -Requested GL_ARB_texture_rectangle ERROR: node is still EOpNull! 0:42 Function Definition: main( ( global void) 0:42 Function Parameters: diff --git a/glslang/MachineIndependent/Intermediate.cpp b/glslang/MachineIndependent/Intermediate.cpp index 0504ac87..17e3fbc2 100755 --- a/glslang/MachineIndependent/Intermediate.cpp +++ b/glslang/MachineIndependent/Intermediate.cpp @@ -1162,18 +1162,18 @@ TIntermTyped* TIntermediate::addConversion(TOperator op, const TType& type, TInt // - at the time of this writing (14-Aug-2020), no test results are changed by this. switch (op) { case EOpConstructFloat16: - canPromoteConstant = extensionRequested(E_GL_EXT_shader_explicit_arithmetic_types) || - extensionRequested(E_GL_EXT_shader_explicit_arithmetic_types_float16); + canPromoteConstant = numericFeatures.contains(TNumericFeatures::shader_explicit_arithmetic_types) || + numericFeatures.contains(TNumericFeatures::shader_explicit_arithmetic_types_float16); break; case EOpConstructInt8: case EOpConstructUint8: - canPromoteConstant = extensionRequested(E_GL_EXT_shader_explicit_arithmetic_types) || - extensionRequested(E_GL_EXT_shader_explicit_arithmetic_types_int8); + canPromoteConstant = numericFeatures.contains(TNumericFeatures::shader_explicit_arithmetic_types) || + numericFeatures.contains(TNumericFeatures::shader_explicit_arithmetic_types_int8); break; case EOpConstructInt16: case EOpConstructUint16: - canPromoteConstant = extensionRequested(E_GL_EXT_shader_explicit_arithmetic_types) || - extensionRequested(E_GL_EXT_shader_explicit_arithmetic_types_int16); + canPromoteConstant = numericFeatures.contains(TNumericFeatures::shader_explicit_arithmetic_types) || + numericFeatures.contains(TNumericFeatures::shader_explicit_arithmetic_types_int16); break; } #endif @@ -1665,14 +1665,14 @@ bool TIntermediate::canImplicitlyPromote(TBasicType from, TBasicType to, TOperat isFPConversion(from, to) || isFPIntegralConversion(from, to)) { - if (extensionRequested(E_GL_EXT_shader_explicit_arithmetic_types) || - extensionRequested(E_GL_EXT_shader_explicit_arithmetic_types_int8) || - extensionRequested(E_GL_EXT_shader_explicit_arithmetic_types_int16) || - extensionRequested(E_GL_EXT_shader_explicit_arithmetic_types_int32) || - extensionRequested(E_GL_EXT_shader_explicit_arithmetic_types_int64) || - extensionRequested(E_GL_EXT_shader_explicit_arithmetic_types_float16) || - extensionRequested(E_GL_EXT_shader_explicit_arithmetic_types_float32) || - extensionRequested(E_GL_EXT_shader_explicit_arithmetic_types_float64)) { + if (numericFeatures.contains(TNumericFeatures::shader_explicit_arithmetic_types) || + numericFeatures.contains(TNumericFeatures::shader_explicit_arithmetic_types_int8) || + numericFeatures.contains(TNumericFeatures::shader_explicit_arithmetic_types_int16) || + numericFeatures.contains(TNumericFeatures::shader_explicit_arithmetic_types_int32) || + numericFeatures.contains(TNumericFeatures::shader_explicit_arithmetic_types_int64) || + numericFeatures.contains(TNumericFeatures::shader_explicit_arithmetic_types_float16) || + numericFeatures.contains(TNumericFeatures::shader_explicit_arithmetic_types_float32) || + numericFeatures.contains(TNumericFeatures::shader_explicit_arithmetic_types_float64)) { return true; } } @@ -1684,14 +1684,14 @@ bool TIntermediate::canImplicitlyPromote(TBasicType from, TBasicType to, TOperat switch (from) { case EbtInt: case EbtUint: - return extensionRequested(E_GL_EXT_shader_implicit_conversions); + return numericFeatures.contains(TNumericFeatures::shader_implicit_conversions); default: return false; } case EbtUint: switch (from) { case EbtInt: - return extensionRequested(E_GL_EXT_shader_implicit_conversions); + return numericFeatures.contains(TNumericFeatures::shader_implicit_conversions); default: return false; } @@ -1707,14 +1707,14 @@ bool TIntermediate::canImplicitlyPromote(TBasicType from, TBasicType to, TOperat case EbtInt64: case EbtUint64: case EbtFloat: - return version >= 400 || extensionRequested(E_GL_ARB_gpu_shader_fp64); + return version >= 400 || numericFeatures.contains(TNumericFeatures::gpu_shader_fp64); case EbtInt16: case EbtUint16: - return (version >= 400 || extensionRequested(E_GL_ARB_gpu_shader_fp64)) && - extensionRequested(E_GL_AMD_gpu_shader_int16); + return (version >= 400 || numericFeatures.contains(TNumericFeatures::gpu_shader_fp64)) && + numericFeatures.contains(TNumericFeatures::gpu_shader_int16); case EbtFloat16: - return (version >= 400 || extensionRequested(E_GL_ARB_gpu_shader_fp64)) && - extensionRequested(E_GL_AMD_gpu_shader_half_float); + return (version >= 400 || numericFeatures.contains(TNumericFeatures::gpu_shader_fp64)) && + numericFeatures.contains(TNumericFeatures::gpu_shader_half_float); default: return false; } @@ -1727,10 +1727,10 @@ bool TIntermediate::canImplicitlyPromote(TBasicType from, TBasicType to, TOperat return getSource() == EShSourceHlsl; case EbtInt16: case EbtUint16: - return extensionRequested(E_GL_AMD_gpu_shader_int16); + return numericFeatures.contains(TNumericFeatures::gpu_shader_int16); case EbtFloat16: - return - extensionRequested(E_GL_AMD_gpu_shader_half_float) || getSource() == EShSourceHlsl; + return numericFeatures.contains(TNumericFeatures::gpu_shader_half_float) || + getSource() == EShSourceHlsl; default: return false; } @@ -1742,7 +1742,7 @@ bool TIntermediate::canImplicitlyPromote(TBasicType from, TBasicType to, TOperat return getSource() == EShSourceHlsl; case EbtInt16: case EbtUint16: - return extensionRequested(E_GL_AMD_gpu_shader_int16); + return numericFeatures.contains(TNumericFeatures::gpu_shader_int16); default: return false; } @@ -1751,7 +1751,7 @@ bool TIntermediate::canImplicitlyPromote(TBasicType from, TBasicType to, TOperat case EbtBool: return getSource() == EShSourceHlsl; case EbtInt16: - return extensionRequested(E_GL_AMD_gpu_shader_int16); + return numericFeatures.contains(TNumericFeatures::gpu_shader_int16); default: return false; } @@ -1763,7 +1763,7 @@ bool TIntermediate::canImplicitlyPromote(TBasicType from, TBasicType to, TOperat return true; case EbtInt16: case EbtUint16: - return extensionRequested(E_GL_AMD_gpu_shader_int16); + return numericFeatures.contains(TNumericFeatures::gpu_shader_int16); default: return false; } @@ -1772,7 +1772,7 @@ bool TIntermediate::canImplicitlyPromote(TBasicType from, TBasicType to, TOperat case EbtInt: return true; case EbtInt16: - return extensionRequested(E_GL_AMD_gpu_shader_int16); + return numericFeatures.contains(TNumericFeatures::gpu_shader_int16); default: return false; } @@ -1780,7 +1780,7 @@ bool TIntermediate::canImplicitlyPromote(TBasicType from, TBasicType to, TOperat switch (from) { case EbtInt16: case EbtUint16: - return extensionRequested(E_GL_AMD_gpu_shader_int16); + return numericFeatures.contains(TNumericFeatures::gpu_shader_int16); default: break; } @@ -1788,7 +1788,7 @@ bool TIntermediate::canImplicitlyPromote(TBasicType from, TBasicType to, TOperat case EbtUint16: switch (from) { case EbtInt16: - return extensionRequested(E_GL_AMD_gpu_shader_int16); + return numericFeatures.contains(TNumericFeatures::gpu_shader_int16); default: break; } @@ -1921,7 +1921,7 @@ std::tuple TIntermediate::getConversionDestinationType(T TBasicType res1 = EbtNumTypes; if ((isEsProfile() && - (version < 310 || !extensionRequested(E_GL_EXT_shader_implicit_conversions))) || + (version < 310 || !numericFeatures.contains(TNumericFeatures::shader_implicit_conversions))) || version == 110) return std::make_tuple(res0, res1); diff --git a/glslang/MachineIndependent/ParseHelper.cpp b/glslang/MachineIndependent/ParseHelper.cpp index 2c4b0869..045c8f3e 100644 --- a/glslang/MachineIndependent/ParseHelper.cpp +++ b/glslang/MachineIndependent/ParseHelper.cpp @@ -7316,6 +7316,8 @@ TIntermTyped* TParseContext::constructBuiltIn(const TType& type, TOperator op, T if (!node->getType().isCoopMat()) { if (type.getBasicType() != node->getType().getBasicType()) { node = intermediate.addConversion(type.getBasicType(), node); + if (node == nullptr) + return nullptr; } node = intermediate.setAggregateOperator(node, EOpConstructCooperativeMatrix, type, node->getLoc()); } else { diff --git a/glslang/MachineIndependent/Versions.cpp b/glslang/MachineIndependent/Versions.cpp index b3110241..7469ef15 100644 --- a/glslang/MachineIndependent/Versions.cpp +++ b/glslang/MachineIndependent/Versions.cpp @@ -762,7 +762,8 @@ bool TParseVersions::checkExtensionsRequested(const TSourceLoc& loc, int numExte // Use when there are no profile/version to check, it's just an error if one of the // extensions is not present. // -void TParseVersions::requireExtensions(const TSourceLoc& loc, int numExtensions, const char* const extensions[], const char* featureDesc) +void TParseVersions::requireExtensions(const TSourceLoc& loc, int numExtensions, const char* const extensions[], + const char* featureDesc) { if (checkExtensionsRequested(loc, numExtensions, extensions, featureDesc)) return; @@ -781,7 +782,8 @@ void TParseVersions::requireExtensions(const TSourceLoc& loc, int numExtensions, // Use by preprocessor when there are no profile/version to check, it's just an error if one of the // extensions is not present. // -void TParseVersions::ppRequireExtensions(const TSourceLoc& loc, int numExtensions, const char* const extensions[], const char* featureDesc) +void TParseVersions::ppRequireExtensions(const TSourceLoc& loc, int numExtensions, const char* const extensions[], + const char* featureDesc) { if (checkExtensionsRequested(loc, numExtensions, extensions, featureDesc)) return; @@ -847,6 +849,7 @@ void TParseVersions::updateExtensionBehavior(int line, const char* extension, co error(getCurrentLoc(), "behavior not supported:", "#extension", behaviorString); return; } + bool on = behavior != EBhDisable; // check if extension is used with correct shader stage checkExtensionStage(getCurrentLoc(), extension); @@ -916,6 +919,32 @@ void TParseVersions::updateExtensionBehavior(int line, const char* extension, co updateExtensionBehavior(line, "GL_EXT_shader_explicit_arithmetic_types_int64", behaviorString); else if (strcmp(extension, "GL_EXT_shader_subgroup_extended_types_float16") == 0) updateExtensionBehavior(line, "GL_EXT_shader_explicit_arithmetic_types_float16", behaviorString); + + // see if we need to update the numeric features + else if (strcmp(extension, "GL_EXT_shader_explicit_arithmetic_types") == 0) + intermediate.updateNumericFeature(TNumericFeatures::shader_explicit_arithmetic_types, on); + else if (strcmp(extension, "GL_EXT_shader_explicit_arithmetic_types_int8") == 0) + intermediate.updateNumericFeature(TNumericFeatures::shader_explicit_arithmetic_types_int8, on); + else if (strcmp(extension, "GL_EXT_shader_explicit_arithmetic_types_int16") == 0) + intermediate.updateNumericFeature(TNumericFeatures::shader_explicit_arithmetic_types_int16, on); + else if (strcmp(extension, "GL_EXT_shader_explicit_arithmetic_types_int32") == 0) + intermediate.updateNumericFeature(TNumericFeatures::shader_explicit_arithmetic_types_int32, on); + else if (strcmp(extension, "GL_EXT_shader_explicit_arithmetic_types_int64") == 0) + intermediate.updateNumericFeature(TNumericFeatures::shader_explicit_arithmetic_types_int64, on); + else if (strcmp(extension, "GL_EXT_shader_explicit_arithmetic_types_float16") == 0) + intermediate.updateNumericFeature(TNumericFeatures::shader_explicit_arithmetic_types_float16, on); + else if (strcmp(extension, "GL_EXT_shader_explicit_arithmetic_types_float32") == 0) + intermediate.updateNumericFeature(TNumericFeatures::shader_explicit_arithmetic_types_float32, on); + else if (strcmp(extension, "GL_EXT_shader_explicit_arithmetic_types_float64") == 0) + intermediate.updateNumericFeature(TNumericFeatures::shader_explicit_arithmetic_types_float64, on); + else if (strcmp(extension, "GL_EXT_shader_implicit_conversions") == 0) + intermediate.updateNumericFeature(TNumericFeatures::shader_implicit_conversions, on); + else if (strcmp(extension, "GL_ARB_gpu_shader_fp64") == 0) + intermediate.updateNumericFeature(TNumericFeatures::gpu_shader_fp64, on); + else if (strcmp(extension, "GL_AMD_gpu_shader_int16") == 0) + intermediate.updateNumericFeature(TNumericFeatures::gpu_shader_int16, on); + else if (strcmp(extension, "GL_AMD_gpu_shader_half_float") == 0) + intermediate.updateNumericFeature(TNumericFeatures::gpu_shader_half_float, on); } void TParseVersions::updateExtensionBehavior(const char* extension, TExtensionBehavior behavior) @@ -951,8 +980,8 @@ void TParseVersions::updateExtensionBehavior(const char* extension, TExtensionBe } else { if (iter->second == EBhDisablePartial) warn(getCurrentLoc(), "extension is only partially supported:", "#extension", extension); - if (behavior == EBhEnable || behavior == EBhRequire || behavior == EBhDisable) - intermediate.updateRequestedExtension(extension, behavior); + if (behavior != EBhDisable) + intermediate.addRequestedExtension(extension); iter->second = behavior; } } diff --git a/glslang/MachineIndependent/intermOut.cpp b/glslang/MachineIndependent/intermOut.cpp index 1b409df7..f23a7058 100644 --- a/glslang/MachineIndependent/intermOut.cpp +++ b/glslang/MachineIndependent/intermOut.cpp @@ -1466,7 +1466,7 @@ void TIntermediate::output(TInfoSink& infoSink, bool tree) infoSink.debug << "Shader version: " << version << "\n"; if (requestedExtensions.size() > 0) { for (auto extIt = requestedExtensions.begin(); extIt != requestedExtensions.end(); ++extIt) - infoSink.debug << "Requested " << extIt->first << "\n"; + infoSink.debug << "Requested " << *extIt << "\n"; } if (xfbMode) diff --git a/glslang/MachineIndependent/localintermediate.h b/glslang/MachineIndependent/localintermediate.h index 4345a092..3cdc1f10 100644 --- a/glslang/MachineIndependent/localintermediate.h +++ b/glslang/MachineIndependent/localintermediate.h @@ -233,6 +233,31 @@ private: TMap maps[EsiCount]; }; +class TNumericFeatures { +public: + TNumericFeatures() : features(0) { } + TNumericFeatures(const TNumericFeatures&) = delete; + TNumericFeatures& operator=(const TNumericFeatures&) = delete; + typedef enum : unsigned int { + shader_explicit_arithmetic_types = 1 << 0, + shader_explicit_arithmetic_types_int8 = 1 << 1, + shader_explicit_arithmetic_types_int16 = 1 << 2, + shader_explicit_arithmetic_types_int32 = 1 << 3, + shader_explicit_arithmetic_types_int64 = 1 << 4, + shader_explicit_arithmetic_types_float16 = 1 << 5, + shader_explicit_arithmetic_types_float32 = 1 << 6, + shader_explicit_arithmetic_types_float64 = 1 << 7, + shader_implicit_conversions = 1 << 8, + gpu_shader_fp64 = 1 << 9, + gpu_shader_int16 = 1 << 10, + gpu_shader_half_float = 1 << 11, + } feature; + void insert(feature f) { features |= f; } + void erase(feature f) { features &= ~f; } + bool contains(feature f) const { return (features & f) != 0; } +private: + unsigned int features; +}; // // Set of helper functions to help parse and build the tree. @@ -371,15 +396,8 @@ public: } const SpvVersion& getSpv() const { return spvVersion; } EShLanguage getStage() const { return language; } - void updateRequestedExtension(const char* extension, TExtensionBehavior behavior) { - if(requestedExtensions.find(extension) != requestedExtensions.end()) { - requestedExtensions[extension] = behavior; - } else { - requestedExtensions.insert(std::make_pair(extension, behavior)); - } - } - - const std::map& getRequestedExtensions() const { return requestedExtensions; } + void addRequestedExtension(const char* extension) { requestedExtensions.insert(extension); } + const std::set& getRequestedExtensions() const { return requestedExtensions; } void setTreeRoot(TIntermNode* r) { treeRoot = r; } TIntermNode* getTreeRoot() const { return treeRoot; } @@ -867,22 +885,25 @@ public: bool getArithemeticInt8Enabled() const { return false; } bool getArithemeticInt16Enabled() const { return false; } bool getArithemeticFloat16Enabled() const { return false; } + void updateNumericFeature(TNumericFeatures::feature f, bool on) { } #else bool getArithemeticInt8Enabled() const { - return extensionRequested(E_GL_EXT_shader_explicit_arithmetic_types) || - extensionRequested(E_GL_EXT_shader_explicit_arithmetic_types_int8); + return numericFeatures.contains(TNumericFeatures::shader_explicit_arithmetic_types) || + numericFeatures.contains(TNumericFeatures::shader_explicit_arithmetic_types_int8); } bool getArithemeticInt16Enabled() const { - return extensionRequested(E_GL_EXT_shader_explicit_arithmetic_types) || - extensionRequested(E_GL_AMD_gpu_shader_int16) || - extensionRequested(E_GL_EXT_shader_explicit_arithmetic_types_int16); + return numericFeatures.contains(TNumericFeatures::shader_explicit_arithmetic_types) || + numericFeatures.contains(TNumericFeatures::gpu_shader_int16) || + numericFeatures.contains(TNumericFeatures::shader_explicit_arithmetic_types_int16); } bool getArithemeticFloat16Enabled() const { - return extensionRequested(E_GL_EXT_shader_explicit_arithmetic_types) || - extensionRequested(E_GL_AMD_gpu_shader_half_float) || - extensionRequested(E_GL_EXT_shader_explicit_arithmetic_types_float16); + return numericFeatures.contains(TNumericFeatures::shader_explicit_arithmetic_types) || + numericFeatures.contains(TNumericFeatures::gpu_shader_half_float) || + numericFeatures.contains(TNumericFeatures::shader_explicit_arithmetic_types_float16); } + void updateNumericFeature(TNumericFeatures::feature f, bool on) + { on ? numericFeatures.insert(f) : numericFeatures.erase(f); } #endif protected: @@ -916,22 +937,6 @@ protected: bool isConversionAllowed(TOperator op, TIntermTyped* node) const; std::tuple getConversionDestinationType(TBasicType type0, TBasicType type1, TOperator op) const; - // JohnK: I think this function should go away. - // This data structure is just a log to pass on to back ends. - // Versioning and extensions are handled in Version.cpp, with a rich - // set of functions for querying stages, versions, extension enable/disabled, etc. -#ifdef GLSLANG_WEB - bool extensionRequested(const char *extension) const { return false; } -#else - bool extensionRequested(const char *extension) const { - auto it = requestedExtensions.find(extension); - if (it != requestedExtensions.end()) { - return (it->second == EBhDisable) ? false : true; - } - return false; - } -#endif - static const char* getResourceName(TResourceType); const EShLanguage language; // stage, known at construction time @@ -949,7 +954,7 @@ protected: #endif SpvVersion spvVersion; TIntermNode* treeRoot; - std::map requestedExtensions; // cumulation of all enabled or required extensions; not connected to what subset of the shader used them + std::set requestedExtensions; // cumulation of all enabled or required extensions; not connected to what subset of the shader used them TBuiltInResource resources; int numEntryPoints; int numErrors; @@ -1020,6 +1025,7 @@ protected: std::unordered_map uniformLocationOverrides; int uniformLocationBase; + TNumericFeatures numericFeatures; #endif std::unordered_set usedConstantId; // specialization constant ids used diff --git a/glslang/MachineIndependent/parseVersions.h b/glslang/MachineIndependent/parseVersions.h index 957aaefd..7248354e 100644 --- a/glslang/MachineIndependent/parseVersions.h +++ b/glslang/MachineIndependent/parseVersions.h @@ -229,7 +229,7 @@ public: TIntermediate& intermediate; // helper for making and hooking up pieces of the parse tree protected: - TMap extensionBehavior; // for each extension string, what its current behavior is set to + TMap extensionBehavior; // for each extension string, what its current behavior is TMap extensionMinSpv; // for each extension string, store minimum spirv required EShMessages messages; // errors/warnings/rule-sets int numErrors; // number of compile-time errors encountered