Add cap on intel to avoid calling abs and floor on the same line in a

shader.

This fixes a bug on some intel devices where we are failing
in the ProcessorOptimizationTest.

I've tried other "no op" type things between the floor call and abs which
also fixed the issue, as well as adding explicit checks to see if we are
less than -1 or greater than 1 where the clamp is. Thus the clamp itself
should be a no op and shouldn't secretly be fixing the problem outside
of forcing the floor and abs lines to be separate.

Bug: skia:
Change-Id: I85bf82e0e02607b78470b7a5f8f918e9f53f0154
Reviewed-on: https://skia-review.googlesource.com/76820
Reviewed-by: Robert Phillips <robertphillips@google.com>
Reviewed-by: Brian Salomon <bsalomon@google.com>
Commit-Queue: Greg Daniel <egdaniel@google.com>

6 files changed, 26 insertions, 4 deletions

diff --git a/include/gpu/GrShaderCaps.h b/include/gpu/GrShaderCaps.h
index 15377ff0b1..e912e146de 100644
--- a/include/gpu/GrShaderCaps.h
+++ b/include/gpu/GrShaderCaps.h
@@ -116,6 +116,11 @@ public:
     // Returns whether a device incorrectly implements atan(y,x) as atan(y/x)
     bool atan2ImplementedAsAtanYOverX() const { return fAtan2ImplementedAsAtanYOverX; }
 
+    // If this returns true some operation (could be a no op) must be called between floor and abs
+    // to make sure the driver compiler doesn't inline them together which can cause a driver bug in
+    // the shader.
+    bool mustDoOpBetweenFloorAndAbs() const { return fMustDoOpBetweenFloorAndAbs; }
+
     bool requiresLocalOutputColorForFBFetch() const { return fRequiresLocalOutputColorForFBFetch; }
 
     bool mustObfuscateUniformColor() const { return fMustObfuscateUniformColor; }
@@ -258,6 +263,7 @@ private:
     bool fCanUseFractForNegativeValues : 1;
     bool fMustForceNegatedAtanParamToFloat : 1;
     bool fAtan2ImplementedAsAtanYOverX : 1;
+    bool fMustDoOpBetweenFloorAndAbs : 1;
     bool fRequiresLocalOutputColorForFBFetch : 1;
     bool fMustObfuscateUniformColor : 1;
     bool fMustGuardDivisionEvenAfterExplicitZeroCheck : 1;
diff --git a/src/gpu/GrShaderCaps.cpp b/src/gpu/GrShaderCaps.cpp
index 65721ee897..e232f3d826 100644
--- a/src/gpu/GrShaderCaps.cpp
+++ b/src/gpu/GrShaderCaps.cpp
@@ -34,6 +34,7 @@ GrShaderCaps::GrShaderCaps(const GrContextOptions& options) {
     fCanUseFractForNegativeValues = true;
     fMustForceNegatedAtanParamToFloat = false;
     fAtan2ImplementedAsAtanYOverX = false;
+    fMustDoOpBetweenFloorAndAbs = false;
     fRequiresLocalOutputColorForFBFetch = false;
     fMustObfuscateUniformColor = false;
     fMustGuardDivisionEvenAfterExplicitZeroCheck = false;
@@ -106,6 +107,7 @@ void GrShaderCaps::dumpJSON(SkJSONWriter* writer) const {
     writer->appendBool("Can use min() and abs() together", fCanUseMinAndAbsTogether);
     writer->appendBool("Can use fract() for negative values", fCanUseFractForNegativeValues);
     writer->appendBool("Must force negated atan param to float", fMustForceNegatedAtanParamToFloat);
+    writer->appendBool("Must do op between floor and abs", fMustDoOpBetweenFloorAndAbs);
     writer->appendBool("Must use local out color for FBFetch", fRequiresLocalOutputColorForFBFetch);
     writer->appendBool("Must obfuscate uniform color", fMustObfuscateUniformColor);
     writer->appendBool("Must guard division even after explicit zero check",
diff --git a/src/gpu/gl/GrGLCaps.cpp b/src/gpu/gl/GrGLCaps.cpp
index a31b0c0daa..d0436a43f2 100644
--- a/src/gpu/gl/GrGLCaps.cpp
+++ b/src/gpu/gl/GrGLCaps.cpp
@@ -1000,6 +1000,13 @@ void GrGLCaps::initGLSL(const GrGLContextInfo& ctxInfo, const GrGLInterface* gli
         shaderCaps->fMustForceNegatedAtanParamToFloat = true;
     }
 
+    // On some Intel GPUs there is an issue where the driver outputs bogus values in the shader
+    // when floor and abs are called on the same line. Thus we must execute an Op between them to
+    // make sure the compiler doesn't re-inline them even if we break the calls apart.
+    if (kIntel_GrGLVendor == ctxInfo.vendor()) {
+        shaderCaps->fMustDoOpBetweenFloorAndAbs = true;
+    }
+
     // On Adreno devices with framebuffer fetch support, there is a bug where they always return
     // the original dst color when reading the outColor even after being written to. By using a
     // local outColor we can work around this bug.
diff --git a/src/shaders/gradients/SkGradientShader.cpp b/src/shaders/gradients/SkGradientShader.cpp
index c448b4038a..1ff42e7a2f 100644
--- a/src/shaders/gradients/SkGradientShader.cpp
+++ b/src/shaders/gradients/SkGradientShader.cpp
@@ -1052,7 +1052,14 @@ void GrGradientEffect::GLSLProcessor::emitAnalyticalColor(GrGLSLFPFragmentBuilde
             break;
         case GrSamplerState::WrapMode::kMirrorRepeat:
             fragBuilder->codeAppendf("half t_1 = %s - 1.0;", t);
-            fragBuilder->codeAppendf("half tiled_t = abs(t_1 - 2.0 * floor(t_1 * 0.5) - 1.0);");
+            fragBuilder->codeAppendf("half tiled_t = t_1 - 2.0 * floor(t_1 * 0.5) - 1.0;");
+            if (shaderCaps->mustDoOpBetweenFloorAndAbs()) {
+                // At this point the expected value of tiled_t should between -1 and 1, so this
+                // clamp has no effect other than to break up the floor and abs calls and make sure
+                // the compiler doesn't merge them back together.
+                fragBuilder->codeAppendf("tiled_t = clamp(tiled_t, -1.0, 1.0);");
+            }
+            fragBuilder->codeAppendf("tiled_t = abs(tiled_t);");
             break;
     }
 
diff --git a/src/shaders/gradients/SkTwoPointConicalGradient_gpu.cpp b/src/shaders/gradients/SkTwoPointConicalGradient_gpu.cpp
index fec4373a33..a11a466976 100644
--- a/src/shaders/gradients/SkTwoPointConicalGradient_gpu.cpp
+++ b/src/shaders/gradients/SkTwoPointConicalGradient_gpu.cpp
@@ -274,7 +274,7 @@ void Edge2PtConicalEffect::GLSLEdge2PtConicalProcessor::emitCode(EmitArgs& args)
 
     // if r(t) > 0, then t will be the x coordinate
     fragBuilder->codeAppendf("\tif (%s * %s + %s > 0.0) {\n", tName.c_str(),
-                           p2.c_str(), p0.c_str());
+                             p2.c_str(), p0.c_str());
     fragBuilder->codeAppend("\t");
     this->emitColor(fragBuilder,
                     uniformHandler,
diff --git a/tests/ProcessorTest.cpp b/tests/ProcessorTest.cpp
index 177eead450..1d393d6b79 100644
--- a/tests/ProcessorTest.cpp
+++ b/tests/ProcessorTest.cpp
@@ -434,8 +434,8 @@ DEF_GPUTEST_FOR_GL_RENDERING_CONTEXTS(ProcessorOptimizationValidationTest, repor
                         if (!legalColorModulation && !legalAlphaModulation) {
                             ERRORF(reporter,
                                    "\"Modulating\" processor %s made color/alpha value larger. "
-                                   "Input: 0x%08x, Output: 0x%08x.",
-                                   clone->name(), input, output);
+                                   "Input: 0x%08x, Output: 0x%08x, pixel (%d, %d).",
+                                   clone->name(), input, output, x, y);
                             passing = false;
                         }
                     }