From 2bd48f58ff57c7757cf0c778708af7ce40f77de2 Mon Sep 17 00:00:00 2001 From: Eliza Velasquez Date: Tue, 7 Nov 2023 13:03:24 -0800 Subject: [PATCH] Enable optimizations for ESSL 1.0 code The CL introducing the ESSL 1.0 chunk in materials inadvertently disabled optimizations for said code. This commit reintroduces those optimizations and fixes associated bugs which manifested. In particular, spirv-cross was generating uints for bools; this has been fixed with a hack. Additionally, spirv-cross is now compiled with exceptions enabled so that matc can gracefully fail and show the code which failed to compile rather than abruptly aborting. --- CMakeLists.txt | 6 ++++++ NEW_RELEASE_NOTES.md | 2 +- libs/filamat/src/GLSLPostProcessor.cpp | 21 ++++++++++++++------- libs/filamat/src/GLSLPostProcessor.h | 2 +- libs/filamat/src/MaterialBuilder.cpp | 9 ++++----- libs/filamat/src/shaders/CodeGenerator.cpp | 16 ++++++++++------ third_party/spirv-cross/spirv_glsl.cpp | 13 +++++++++++-- 7 files changed, 47 insertions(+), 22 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 00e03a598fe..33c2bb29685 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -67,6 +67,9 @@ set(FILAMENT_METAL_HANDLE_ARENA_SIZE_IN_MB "8" CACHE STRING "Size of the Metal handle arena, default 8." ) +# Enable exceptions by default in spirv-cross. +set(SPIRV_CROSS_EXCEPTIONS_TO_ASSERTIONS OFF) + # ================================================================================================== # CMake policies # ================================================================================================== @@ -339,6 +342,7 @@ endif() if (CYGWIN) set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -fno-exceptions -fno-rtti") + set(SPIRV_CROSS_EXCEPTIONS_TO_ASSERTIONS ON) endif() if (MSVC) @@ -375,6 +379,7 @@ endif() # saved by -fno-exception and 10 KiB saved by -fno-rtti). if (ANDROID OR IOS OR WEBGL) set(CMAKE_CXX_FLAGS_RELEASE "${CMAKE_CXX_FLAGS_RELEASE} -fno-exceptions -fno-rtti") + set(SPIRV_CROSS_EXCEPTIONS_TO_ASSERTIONS ON) if (ANDROID OR WEBGL) # Omitting unwind info prevents the generation of readable stack traces in crash reports on iOS @@ -386,6 +391,7 @@ endif() # std::visit, which is not supported on iOS 11.0 when exceptions are enabled. if (IOS) set(CMAKE_CXX_FLAGS_DEBUG "${CMAKE_CXX_FLAGS_DEBUG} -fno-exceptions") + set(SPIRV_CROSS_EXCEPTIONS_TO_ASSERTIONS ON) endif() # With WebGL, we disable RTTI even for debug builds because we pass emscripten::val back and forth diff --git a/NEW_RELEASE_NOTES.md b/NEW_RELEASE_NOTES.md index 19887bf7a8b..d000a526635 100644 --- a/NEW_RELEASE_NOTES.md +++ b/NEW_RELEASE_NOTES.md @@ -9,4 +9,4 @@ appropriate header in [RELEASE_NOTES.md](./RELEASE_NOTES.md). ## Release notes for next branch cut - matc: New option `-1` to disable generation of ESSL 1.0 code in Feature Level 0 materials -- matc: Enable preprocessor optimization of ESSL 1.0 shader code [⚠️ **Recompile materials**] +- matc: Support optimizations for ESSL 1.0 code [⚠️ **Recompile materials**] diff --git a/libs/filamat/src/GLSLPostProcessor.cpp b/libs/filamat/src/GLSLPostProcessor.cpp index b4aaf54e4a2..7c1c4138fd7 100644 --- a/libs/filamat/src/GLSLPostProcessor.cpp +++ b/libs/filamat/src/GLSLPostProcessor.cpp @@ -33,6 +33,7 @@ #include "MetalArgumentBuffer.h" #include "SpirvFixup.h" +#include "utils/ostream.h" #include @@ -396,12 +397,8 @@ bool GLSLPostProcessor::process(const std::string& inputShader, Config const& co break; case MaterialBuilder::Optimization::SIZE: case MaterialBuilder::Optimization::PERFORMANCE: - if (config.featureLevel == filament::backend::FeatureLevel::FEATURE_LEVEL_0) { - // Full optimization blocked by upstream issue: - // https://github.com/KhronosGroup/SPIRV-Cross/issues/2223 - preprocessOptimization(tShader, config, internalConfig); - } else { - fullOptimization(tShader, config, internalConfig); + if (!fullOptimization(tShader, config, internalConfig)) { + return false; } break; } @@ -485,7 +482,7 @@ void GLSLPostProcessor::preprocessOptimization(glslang::TShader& tShader, } } -void GLSLPostProcessor::fullOptimization(const TShader& tShader, +bool GLSLPostProcessor::fullOptimization(const TShader& tShader, GLSLPostProcessor::Config const& config, InternalConfig& internalConfig) const { SpirvBlob spirv; @@ -553,7 +550,16 @@ void GLSLPostProcessor::fullOptimization(const TShader& tShader, } } +#ifdef SPIRV_CROSS_EXCEPTIONS_TO_ASSERTIONS *internalConfig.glslOutput = glslCompiler.compile(); +#else + try { + *internalConfig.glslOutput = glslCompiler.compile(); + } catch (spirv_cross::CompilerError e) { + slog.e << "ERROR: " << e.what() << io::endl; + return false; + } +#endif // spirv-cross automatically redeclares gl_ClipDistance if it's used. Some drivers don't // like this, so we simply remove it. @@ -566,6 +572,7 @@ void GLSLPostProcessor::fullOptimization(const TShader& tShader, str.replace(found, clipDistanceDefinition.length(), ""); } } + return true; } std::shared_ptr GLSLPostProcessor::createOptimizer( diff --git a/libs/filamat/src/GLSLPostProcessor.h b/libs/filamat/src/GLSLPostProcessor.h index 8e3c2e1e4ac..c13dece6369 100644 --- a/libs/filamat/src/GLSLPostProcessor.h +++ b/libs/filamat/src/GLSLPostProcessor.h @@ -93,7 +93,7 @@ class GLSLPostProcessor { ShaderMinifier minifier; }; - void fullOptimization(const glslang::TShader& tShader, + bool fullOptimization(const glslang::TShader& tShader, GLSLPostProcessor::Config const& config, InternalConfig& internalConfig) const; void preprocessOptimization(glslang::TShader& tShader, diff --git a/libs/filamat/src/MaterialBuilder.cpp b/libs/filamat/src/MaterialBuilder.cpp index 52cba77d6df..a95bb077819 100644 --- a/libs/filamat/src/MaterialBuilder.cpp +++ b/libs/filamat/src/MaterialBuilder.cpp @@ -142,11 +142,10 @@ void MaterialBuilderBase::prepare(bool vulkanSemantics, if (mIncludeEssl1 && featureLevel == filament::backend::FeatureLevel::FEATURE_LEVEL_0 && shaderModel == ShaderModel::MOBILE) { - // ESSL1 code may never be compiled to SPIR-V. mCodeGenPermutations.push_back({ shaderModel, TargetApi::OPENGL, - TargetLanguage::GLSL, + glTargetLanguage, filament::backend::FeatureLevel::FEATURE_LEVEL_0 }); } @@ -1422,15 +1421,15 @@ void MaterialBuilder::writeCommonChunks(ChunkContainer& container, MaterialInfo& uniforms.push_back({ "objectUniforms.data[0].morphTargetCount", offsetof(PerRenderableUib, data[0].morphTargetCount), 1, - UniformType::UINT }); + UniformType::INT }); uniforms.push_back({ "objectUniforms.data[0].flagsChannels", offsetof(PerRenderableUib, data[0].flagsChannels), 1, - UniformType::UINT }); + UniformType::INT }); uniforms.push_back({ "objectUniforms.data[0].objectId", offsetof(PerRenderableUib, data[0].objectId), 1, - UniformType::UINT }); + UniformType::INT }); uniforms.push_back({ "objectUniforms.data[0].userData", offsetof(PerRenderableUib, data[0].userData), 1, diff --git a/libs/filamat/src/shaders/CodeGenerator.cpp b/libs/filamat/src/shaders/CodeGenerator.cpp index a5b48ff7c8b..2c20307b7f4 100644 --- a/libs/filamat/src/shaders/CodeGenerator.cpp +++ b/libs/filamat/src/shaders/CodeGenerator.cpp @@ -301,8 +301,9 @@ utils::io::sstream& CodeGenerator::generateProlog(utils::io::sstream& out, Shade out << '\n'; out << SHADERS_COMMON_DEFINES_GLSL_DATA; - if (mFeatureLevel > FeatureLevel::FEATURE_LEVEL_0 && - material.featureLevel == FeatureLevel::FEATURE_LEVEL_0) { + if (material.featureLevel == FeatureLevel::FEATURE_LEVEL_0 && + (mFeatureLevel > FeatureLevel::FEATURE_LEVEL_0 + || mTargetLanguage == TargetLanguage::SPIRV)) { // Insert compatibility definitions for ESSL 1.0 functions which were removed in ESSL 3.0. // This is the minimum required value according to the OpenGL ES Shading Language Version @@ -490,11 +491,14 @@ io::sstream& CodeGenerator::generateOutput(io::sstream& out, ShaderStage type, const char* materialTypeString = getOutputTypeName(materialOutputType); const char* typeString = getOutputTypeName(outputType); + bool generate_essl3_code = mTargetLanguage == TargetLanguage::SPIRV + || mFeatureLevel >= FeatureLevel::FEATURE_LEVEL_1; + out << "\n#define FRAG_OUTPUT" << index << " " << name.c_str(); - if (mFeatureLevel == FeatureLevel::FEATURE_LEVEL_0) { - out << "\n#define FRAG_OUTPUT_AT" << index << " gl_FragColor"; - } else { + if (generate_essl3_code) { out << "\n#define FRAG_OUTPUT_AT" << index << " output_" << name.c_str(); + } else { + out << "\n#define FRAG_OUTPUT_AT" << index << " gl_FragColor"; } out << "\n#define FRAG_OUTPUT_MATERIAL_TYPE" << index << " " << materialTypeString; out << "\n#define FRAG_OUTPUT_PRECISION" << index << " " << precisionString; @@ -502,7 +506,7 @@ io::sstream& CodeGenerator::generateOutput(io::sstream& out, ShaderStage type, out << "\n#define FRAG_OUTPUT_SWIZZLE" << index << " " << swizzleString; out << "\n"; - if (mFeatureLevel >= FeatureLevel::FEATURE_LEVEL_1) { + if (generate_essl3_code) { out << "\nlayout(location=" << index << ") out " << precisionString << " " << typeString << " output_" << name.c_str() << ";\n"; } diff --git a/third_party/spirv-cross/spirv_glsl.cpp b/third_party/spirv-cross/spirv_glsl.cpp index 0d63d35f8f2..e0b39f95bb7 100644 --- a/third_party/spirv-cross/spirv_glsl.cpp +++ b/third_party/spirv-cross/spirv_glsl.cpp @@ -14977,7 +14977,12 @@ string CompilerGLSL::flags_to_qualifiers_glsl(const SPIRType &type, const Bitset { auto &execution = get_entry_point(); - if (flags.get(DecorationRelaxedPrecision)) + if (type.basetype == SPIRType::UInt && is_legacy_es()) + { + // HACK: This is a bool. See comment in type_to_glsl(). + qual += "lowp "; + } + else if (flags.get(DecorationRelaxedPrecision)) { bool implied_fmediump = type.basetype == SPIRType::Float && options.fragment.default_float_precision == Options::Mediump && @@ -15503,7 +15508,11 @@ string CompilerGLSL::type_to_glsl(const SPIRType &type, uint32_t id) if (type.basetype == SPIRType::UInt && is_legacy()) { if (options.es) - SPIRV_CROSS_THROW("Unsigned integers are not supported on legacy ESSL."); + // HACK: spirv-cross changes bools into uints and generates code which compares them to + // zero. Input code will have already been validated as not to have contained any uints, + // so any remaining uints must in fact be bools. However, simply returning "bool" here + // will result in invalid code. Instead, return an int. + return backend.basic_int_type; else require_extension_internal("GL_EXT_gpu_shader4"); }