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 <yuxinhu@google.com> Reviewed-by: Yiwei Zhang <zzyiwei@chromium.org> Commit-Queue: Shahbaz Youssefi <syoussefi@chromium.org>
2026-01-06 02:09:55 +03:00 · 2023-04-11 15:16:29 -04:00
parent 663b60b5fa
commit c26011b866
11 changed files with 21 additions and 64 deletions
--- 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.
--- 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,
--- 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",
--- 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"
 }
--- 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;
            }
--- 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<ShaderVariable> *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<ShaderVariable> *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<ShaderVariable> *uniforms,
                                   const AdvancedBlendEquations &advancedBlendEquations)
 {
-    Builder builder(compiler, compileOptions, symbolTable, driverUniforms, uniforms,
-                    advancedBlendEquations);
+    Builder builder(compiler, symbolTable, driverUniforms, uniforms, advancedBlendEquations);
    return builder.build(root);
 }  // namespace

--- 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,
--- 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.
--- a/src/libANGLE/renderer/vulkan/ShaderVk.cpp
+++ b/src/libANGLE/renderer/vulkan/ShaderVk.cpp
@@ -103,11 +103,6 @@ std::shared_ptr<WaitableCompileEvent> 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;
--- a/util/angle_features_autogen.cpp
+++ b/util/angle_features_autogen.cpp
@@ -201,7 +201,6 @@ constexpr PackedEnumMap<Feature, const char *> kFeatureNames = {{
    {Feature::PermanentlySwitchToFramebufferFetchMode, "permanentlySwitchToFramebufferFetchMode"},
    {Feature::PersistentlyMappedBuffers, "persistentlyMappedBuffers"},
    {Feature::PreAddTexelFetchOffsets, "preAddTexelFetchOffsets"},
-    {Feature::PrecisionSafeDivision, "precisionSafeDivision"},
    {Feature::PreemptivelyStartProvokingVertexCommandBuffer,
     "preemptivelyStartProvokingVertexCommandBuffer"},
    {Feature::PreferAggregateBarrierCalls, "preferAggregateBarrierCalls"},
--- 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,