From c26011b8663fc9e25f3a08a0f1be65f4cc1cb320 Mon Sep 17 00:00:00 2001 From: Shahbaz Youssefi Date: Tue, 11 Apr 2023 15:16:29 -0400 Subject: [PATCH] Vulkan: Make advanced blend alpha div workaround permanent This workaround turned out to affect pretty much every driver, and numerous GL implementations were found to work around it similarly to ANGLE. It seems like this workaround may only be necessary for colorburn and colordodge, but for now ANGLE applies it to all modes. Bug: b/274528004 Bug: b/277777623 Change-Id: Id555c981a9775f949a3022b7e92c755accea7cea Reviewed-on: https://chromium-review.googlesource.com/c/angle/angle/+/4416158 Reviewed-by: Yuxin Hu Reviewed-by: Yiwei Zhang Commit-Queue: Shahbaz Youssefi --- include/GLSLANG/ShaderLang.h | 8 ++--- include/platform/FeaturesVk_autogen.h | 8 ----- include/platform/vk_features.json | 8 ----- .../ANGLE_features.json | 8 ++--- src/compiler/translator/TranslatorVulkan.cpp | 5 ++- .../vulkan/EmulateAdvancedBlendEquations.cpp | 36 ++++++------------- .../vulkan/EmulateAdvancedBlendEquations.h | 1 - src/libANGLE/renderer/vulkan/RendererVk.cpp | 4 --- src/libANGLE/renderer/vulkan/ShaderVk.cpp | 5 --- util/angle_features_autogen.cpp | 1 - util/angle_features_autogen.h | 1 - 11 files changed, 21 insertions(+), 64 deletions(-) diff --git a/include/GLSLANG/ShaderLang.h b/include/GLSLANG/ShaderLang.h index bfb0c1419..61fabda4f 100644 --- a/include/GLSLANG/ShaderLang.h +++ b/include/GLSLANG/ShaderLang.h @@ -26,7 +26,7 @@ // Version number for shader translation API. // It is incremented every time the API changes. -#define ANGLE_SH_VERSION 324 +#define ANGLE_SH_VERSION 325 enum ShShaderSpec { @@ -405,9 +405,9 @@ struct ShCompileOptions // ceil()ed instead. uint64_t roundOutputAfterDithering : 1; - // Even when the dividend and divisor have the same value some platforms do not return 1.0f. - // Need to emit different division code for such platforms. - uint64_t precisionSafeDivision : 1; + // Unused. Kept to avoid unnecessarily changing the layout of this structure and tripping up + // the fuzzer's hash->bug map. + uint64_t unused2 : 1; // anglebug.com/7527: packUnorm4x8 fails on Pixel 4 if it is not passed a highp vec4. // TODO(anglebug.com/7527): This workaround is currently only applied for pixel local storage. diff --git a/include/platform/FeaturesVk_autogen.h b/include/platform/FeaturesVk_autogen.h index 49e6d2ed4..c12eb8fd7 100644 --- a/include/platform/FeaturesVk_autogen.h +++ b/include/platform/FeaturesVk_autogen.h @@ -627,14 +627,6 @@ struct FeaturesVk : FeatureSetBase "emulateAdvancedBlendEquations", FeatureCategory::VulkanFeatures, "Emulate GL_KHR_blend_equation_advanced", &members, "http://anglebug.com/3586"}; - FeatureInfo precisionSafeDivision = { - "precisionSafeDivision", - FeatureCategory::VulkanWorkarounds, - "Special case handling for platforms that do not generate 1.0f even when the dividend and " - "divisor have the same value", - &members, - }; - FeatureInfo doubleDepthBiasConstantFactor = { "doubleDepthBiasConstantFactor", FeatureCategory::VulkanWorkarounds, diff --git a/include/platform/vk_features.json b/include/platform/vk_features.json index 1507ca611..6d3264bd0 100644 --- a/include/platform/vk_features.json +++ b/include/platform/vk_features.json @@ -829,14 +829,6 @@ ], "issue": "http://anglebug.com/3586" }, - { - "name": "precision_safe_division", - "category": "Workarounds", - "description": [ - "Special case handling for platforms that do not generate 1.0f even when the dividend and ", - "divisor have the same value" - ] - }, { "name": "double_depth_bias_constant_factor", "category": "Workarounds", diff --git a/scripts/code_generation_hashes/ANGLE_features.json b/scripts/code_generation_hashes/ANGLE_features.json index 7b0e895fe..190058058 100644 --- a/scripts/code_generation_hashes/ANGLE_features.json +++ b/scripts/code_generation_hashes/ANGLE_features.json @@ -6,7 +6,7 @@ "include/platform/FeaturesMtl_autogen.h": "fc2e0328446f01a76c4b301e3b3eae3c", "include/platform/FeaturesVk_autogen.h": - "903665e398db15a03ad664d8a509e36f", + "f493869b097defc3557cee62198a549e", "include/platform/FrontendFeatures_autogen.h": "be41034e621326dd51e86b139705ea39", "include/platform/d3d_features.json": @@ -20,9 +20,9 @@ "include/platform/mtl_features.json": "17c058de70379f2f5695e1791d45d63c", "include/platform/vk_features.json": - "e647c29cbcdc7edd2711b31208379e58", + "d68423837be648217b7c7777fe9519df", "util/angle_features_autogen.cpp": - "1da69692a41738e0f98696be2f55f6e5", + "f219e9cc6facfff6606742031e7a181a", "util/angle_features_autogen.h": - "80fdd754341e09133eee68e5f06cf469" + "988025ab08dfc49b87ec68487cf08af8" } \ No newline at end of file diff --git a/src/compiler/translator/TranslatorVulkan.cpp b/src/compiler/translator/TranslatorVulkan.cpp index 742df0f12..37dc1e5b3 100644 --- a/src/compiler/translator/TranslatorVulkan.cpp +++ b/src/compiler/translator/TranslatorVulkan.cpp @@ -1016,9 +1016,8 @@ bool TranslatorVulkan::translateImpl(TIntermBlock *root, // attachment variable then create a new one. if (getAdvancedBlendEquations().any() && compileOptions.addAdvancedBlendEquationsEmulation && - !EmulateAdvancedBlendEquations(this, compileOptions, root, &getSymbolTable(), - driverUniforms, &mUniforms, - getAdvancedBlendEquations())) + !EmulateAdvancedBlendEquations(this, root, &getSymbolTable(), driverUniforms, + &mUniforms, getAdvancedBlendEquations())) { return false; } diff --git a/src/compiler/translator/tree_ops/vulkan/EmulateAdvancedBlendEquations.cpp b/src/compiler/translator/tree_ops/vulkan/EmulateAdvancedBlendEquations.cpp index 4a4294cff..6d971531c 100644 --- a/src/compiler/translator/tree_ops/vulkan/EmulateAdvancedBlendEquations.cpp +++ b/src/compiler/translator/tree_ops/vulkan/EmulateAdvancedBlendEquations.cpp @@ -33,13 +33,11 @@ class Builder { public: Builder(TCompiler *compiler, - const ShCompileOptions &compileOptions, TSymbolTable *symbolTable, const DriverUniform *driverUniforms, std::vector *uniforms, const AdvancedBlendEquations &advancedBlendEquations) : mCompiler(compiler), - mCompileOptions(compileOptions), mSymbolTable(symbolTable), mDriverUniforms(driverUniforms), mUniforms(uniforms), @@ -60,7 +58,6 @@ class Builder void generateEquationSwitch(TIntermBlock *blendBlock); TCompiler *mCompiler; - const ShCompileOptions &mCompileOptions; TSymbolTable *mSymbolTable; const DriverUniform *mDriverUniforms; std::vector *mUniforms; @@ -1041,32 +1038,23 @@ TIntermSymbol *Builder::premultiplyAlpha(TIntermBlock *blendBlock, TType *vec3Type = new TType(EbtFloat, precision, EvqTemporary, 3); // symbol = vec3(0) - // If alpha != 0 assign symbol based on precisionSafeDivision compile option. + // If alpha != 0, divide by alpha. Note that due to precision issues, component == alpha is + // handled especially. This precision issue affects multiple vendors, and most drivers seem to + // be carrying a similar workaround to pass the CTS test. TIntermTyped *alpha = new TIntermSwizzle(var, {3}); TIntermSymbol *symbol = MakeVariable(mSymbolTable, name, vec3Type); TIntermTyped *alphaNotZero = new TIntermBinary(EOpNotEqual, alpha, Float(0)); TIntermBlock *rgbDivAlphaBlock = new TIntermBlock; constexpr int kColorChannels = 3; - if (mCompileOptions.precisionSafeDivision) + // For each component: + // symbol.x = (var.x == var.w) ? 1.0 : var.x / var.w + for (int index = 0; index < kColorChannels; index++) { - // For each component: - // symbol.x = (var.x == var.w) ? 1.0 : var.x / var.w - for (int index = 0; index < kColorChannels; index++) - { - TIntermTyped *divideNode = divideFloatNode(new TIntermSwizzle(var, {index}), alpha); - TIntermBinary *assignDivideNode = new TIntermBinary( - EOpAssign, new TIntermSwizzle(symbol->deepCopy(), {index}), divideNode); - rgbDivAlphaBlock->appendStatement(assignDivideNode); - } - } - else - { - // symbol = rgb/alpha - TIntermTyped *rgb = new TIntermSwizzle(var->deepCopy(), {0, 1, 2}); - TIntermTyped *rgbDivAlpha = new TIntermBinary(EOpDiv, rgb, alpha->deepCopy()); - rgbDivAlphaBlock->appendStatement( - new TIntermBinary(EOpAssign, symbol->deepCopy(), rgbDivAlpha)); + TIntermTyped *divideNode = divideFloatNode(new TIntermSwizzle(var, {index}), alpha); + TIntermBinary *assignDivideNode = new TIntermBinary( + EOpAssign, new TIntermSwizzle(symbol->deepCopy(), {index}), divideNode); + rgbDivAlphaBlock->appendStatement(assignDivideNode); } TIntermIfElse *ifBlock = new TIntermIfElse(alphaNotZero, rgbDivAlphaBlock, nullptr); @@ -1260,15 +1248,13 @@ void Builder::generateEquationSwitch(TIntermBlock *blendBlock) } // anonymous namespace bool EmulateAdvancedBlendEquations(TCompiler *compiler, - const ShCompileOptions &compileOptions, TIntermBlock *root, TSymbolTable *symbolTable, const DriverUniform *driverUniforms, std::vector *uniforms, const AdvancedBlendEquations &advancedBlendEquations) { - Builder builder(compiler, compileOptions, symbolTable, driverUniforms, uniforms, - advancedBlendEquations); + Builder builder(compiler, symbolTable, driverUniforms, uniforms, advancedBlendEquations); return builder.build(root); } // namespace diff --git a/src/compiler/translator/tree_ops/vulkan/EmulateAdvancedBlendEquations.h b/src/compiler/translator/tree_ops/vulkan/EmulateAdvancedBlendEquations.h index 5631ca89d..70151344d 100644 --- a/src/compiler/translator/tree_ops/vulkan/EmulateAdvancedBlendEquations.h +++ b/src/compiler/translator/tree_ops/vulkan/EmulateAdvancedBlendEquations.h @@ -28,7 +28,6 @@ class AdvancedBlendEquations; // which function to use at runtime. [[nodiscard]] bool EmulateAdvancedBlendEquations( TCompiler *compiler, - const ShCompileOptions &compileOptions, TIntermBlock *root, TSymbolTable *symbolTable, const DriverUniform *driverUniforms, diff --git a/src/libANGLE/renderer/vulkan/RendererVk.cpp b/src/libANGLE/renderer/vulkan/RendererVk.cpp index 3bf276697..acc31b43e 100644 --- a/src/libANGLE/renderer/vulkan/RendererVk.cpp +++ b/src/libANGLE/renderer/vulkan/RendererVk.cpp @@ -4261,10 +4261,6 @@ void RendererVk::initFeatures(DisplayVk *displayVk, &mFeatures, emulateAdvancedBlendEquations, !mFeatures.supportsBlendOperationAdvanced.enabled && (isVenus || !isIntel)); - // Workaround for platforms that do not return 1.0f even when dividend and divisor have the same - // value. - ANGLE_FEATURE_CONDITION(&mFeatures, precisionSafeDivision, isSamsung || isAMD || isVenus); - // http://anglebug.com/6933 // Android expects VkPresentRegionsKHR rectangles with a bottom-left origin, while spec // states they should have a top-left origin. diff --git a/src/libANGLE/renderer/vulkan/ShaderVk.cpp b/src/libANGLE/renderer/vulkan/ShaderVk.cpp index 007665e25..bd4caea52 100644 --- a/src/libANGLE/renderer/vulkan/ShaderVk.cpp +++ b/src/libANGLE/renderer/vulkan/ShaderVk.cpp @@ -103,11 +103,6 @@ std::shared_ptr ShaderVk::compile(const gl::Context *conte options->roundOutputAfterDithering = true; } - if (contextVk->getFeatures().precisionSafeDivision.enabled) - { - options->precisionSafeDivision = true; - } - if (contextVk->getFeatures().appendAliasedMemoryDecorationsToSsbo.enabled) { options->aliasedSSBOUnlessRestrict = true; diff --git a/util/angle_features_autogen.cpp b/util/angle_features_autogen.cpp index f0709a170..13caa10b4 100644 --- a/util/angle_features_autogen.cpp +++ b/util/angle_features_autogen.cpp @@ -201,7 +201,6 @@ constexpr PackedEnumMap kFeatureNames = {{ {Feature::PermanentlySwitchToFramebufferFetchMode, "permanentlySwitchToFramebufferFetchMode"}, {Feature::PersistentlyMappedBuffers, "persistentlyMappedBuffers"}, {Feature::PreAddTexelFetchOffsets, "preAddTexelFetchOffsets"}, - {Feature::PrecisionSafeDivision, "precisionSafeDivision"}, {Feature::PreemptivelyStartProvokingVertexCommandBuffer, "preemptivelyStartProvokingVertexCommandBuffer"}, {Feature::PreferAggregateBarrierCalls, "preferAggregateBarrierCalls"}, diff --git a/util/angle_features_autogen.h b/util/angle_features_autogen.h index c1d6fa699..64f4f3ea5 100644 --- a/util/angle_features_autogen.h +++ b/util/angle_features_autogen.h @@ -191,7 +191,6 @@ enum class Feature PermanentlySwitchToFramebufferFetchMode, PersistentlyMappedBuffers, PreAddTexelFetchOffsets, - PrecisionSafeDivision, PreemptivelyStartProvokingVertexCommandBuffer, PreferAggregateBarrierCalls, PreferCPUForBufferSubData,