Work around atan(y, x) bug on NVIDIA

atan(y, x) is not always returning expected results on NVIDIA OpenGL
drivers between versions 367 and 375. Work around this by emulating
atan(y, x) using the regular atan(x) function. A fix to the driver is
expected in a future release.

It is most convenient to implement the vector atan(y, x) functions by
using the scalar atan(y, x) function. Support for simple dependencies
between emulated functions is added to BuiltInFunctionEmulator. In the
current implementation one function is allowed to have at most one
other function as its dependency.

BUG=chromium:672380
TEST=angle_end2end_tests

Change-Id: I9eba8b0b7979c7c7eaed353b264932e41830beb1
Reviewed-on: https://chromium-review.googlesource.com/419016
Commit-Queue: Olli Etuaho <oetuaho@nvidia.com>
Reviewed-by: Jamie Madill <jmadill@chromium.org>

include/GLSLANG/ShaderLang.h

@@ -207,6 +207,10 @@ const ShCompileOptions SH_USE_UNUSED_STANDARD_SHARED_BLOCKS = UINT64_C(1) << 28;
 // Mac OSX 10.11 drivers. It works by translating -float into 0.0 - float.
 const ShCompileOptions SH_REWRITE_FLOAT_UNARY_MINUS_OPERATOR = UINT64_C(1) << 29;
 
+// This flag works around a bug in evaluating atan(y, x) on some NVIDIA OpenGL drivers.
+// It works by using an expression to emulate this function.
+const ShCompileOptions SH_EMULATE_ATAN2_FLOAT_FUNCTION = UINT64_C(1) << 30;
+
 // Defines alternate strategies for implementing array index clamping.
 enum ShArrayIndexClampingStrategy
 {

src/compiler/translator/BuiltInFunctionEmulator.cpp

@@ -108,29 +108,50 @@ BuiltInFunctionEmulator::BuiltInFunctionEmulator()
 {
 }
 
-void BuiltInFunctionEmulator::addEmulatedFunction(TOperator op,
-                                                  const TType *param,
-                                                  const char *emulatedFunctionDefinition)
+BuiltInFunctionEmulator::FunctionId BuiltInFunctionEmulator::addEmulatedFunction(
+    TOperator op,
+    const TType *param,
+    const char *emulatedFunctionDefinition)
 {
-    mEmulatedFunctions[FunctionId(op, param)] = std::string(emulatedFunctionDefinition);
+    FunctionId id(op, param);
+    mEmulatedFunctions[id] = std::string(emulatedFunctionDefinition);
+    return id;
 }
 
-void BuiltInFunctionEmulator::addEmulatedFunction(TOperator op,
-                                                  const TType *param1,
-                                                  const TType *param2,
-                                                  const char *emulatedFunctionDefinition)
+BuiltInFunctionEmulator::FunctionId BuiltInFunctionEmulator::addEmulatedFunction(
+    TOperator op,
+    const TType *param1,
+    const TType *param2,
+    const char *emulatedFunctionDefinition)
 {
-    mEmulatedFunctions[FunctionId(op, param1, param2)] = std::string(emulatedFunctionDefinition);
+    FunctionId id(op, param1, param2);
+    mEmulatedFunctions[id] = std::string(emulatedFunctionDefinition);
+    return id;
 }
 
-void BuiltInFunctionEmulator::addEmulatedFunction(TOperator op,
-                                                  const TType *param1,
-                                                  const TType *param2,
-                                                  const TType *param3,
-                                                  const char *emulatedFunctionDefinition)
+BuiltInFunctionEmulator::FunctionId BuiltInFunctionEmulator::addEmulatedFunctionWithDependency(
+    FunctionId dependency,
+    TOperator op,
+    const TType *param1,
+    const TType *param2,
+    const char *emulatedFunctionDefinition)
 {
-    mEmulatedFunctions[FunctionId(op, param1, param2, param3)] =
-        std::string(emulatedFunctionDefinition);
+    FunctionId id(op, param1, param2);
+    mEmulatedFunctions[id]    = std::string(emulatedFunctionDefinition);
+    mFunctionDependencies[id] = dependency;
+    return id;
+}
+
+BuiltInFunctionEmulator::FunctionId BuiltInFunctionEmulator::addEmulatedFunction(
+    TOperator op,
+    const TType *param1,
+    const TType *param2,
+    const TType *param3,
+    const char *emulatedFunctionDefinition)
+{
+    FunctionId id(op, param1, param2, param3);
+    mEmulatedFunctions[id] = std::string(emulatedFunctionDefinition);
+    return id;
 }
 
 bool BuiltInFunctionEmulator::IsOutputEmpty() const
@@ -175,6 +196,12 @@ bool BuiltInFunctionEmulator::SetFunctionCalled(const FunctionId &functionId)
             if (mFunctions[i] == functionId)
                 return true;
         }
+        // If the function depends on another, mark the dependency as called.
+        auto dependency = mFunctionDependencies.find(functionId);
+        if (dependency != mFunctionDependencies.end())
+        {
+            SetFunctionCalled((*dependency).second);
+        }
         // Copy the functionId if it needs to be stored, to make sure that the TType pointers inside
         // remain valid and constant.
         mFunctions.push_back(functionId.getCopy());
@@ -197,6 +224,7 @@ void BuiltInFunctionEmulator::MarkBuiltInFunctionsForEmulation(TIntermNode *root
 void BuiltInFunctionEmulator::Cleanup()
 {
     mFunctions.clear();
+    mFunctionDependencies.clear();
 }
 
 // static
@@ -206,6 +234,14 @@ TString BuiltInFunctionEmulator::GetEmulatedFunctionName(const TString &name)
     return "webgl_" + name.substr(0, name.length() - 1) + "_emu(";
 }
 
+BuiltInFunctionEmulator::FunctionId::FunctionId()
+    : mOp(EOpNull),
+      mParam1(TCache::getType(EbtVoid)),
+      mParam2(TCache::getType(EbtVoid)),
+      mParam3(TCache::getType(EbtVoid))
+{
+}
+
 BuiltInFunctionEmulator::FunctionId::FunctionId(TOperator op, const TType *param)
     : mOp(op), mParam1(param), mParam2(TCache::getType(EbtVoid)), mParam3(TCache::getType(EbtVoid))
 {

src/compiler/translator/BuiltInFunctionEmulator.h

@@ -35,40 +35,17 @@ class BuiltInFunctionEmulator
     // Output function emulation definition. This should be before any other shader source.
     void OutputEmulatedFunctions(TInfoSinkBase &out) const;
 
-    // Add functions that need to be emulated.
-    void addEmulatedFunction(TOperator op,
-                             const TType *param,
-                             const char *emulatedFunctionDefinition);
-    void addEmulatedFunction(TOperator op,
-                             const TType *param1,
-                             const TType *param2,
-                             const char *emulatedFunctionDefinition);
-    void addEmulatedFunction(TOperator op,
-                             const TType *param1,
-                             const TType *param2,
-                             const TType *param3,
-                             const char *emulatedFunctionDefinition);
-
-  private:
-    class BuiltInFunctionEmulationMarker;
-
-    // Records that a function is called by the shader and might need to be emulated. If the
-    // function is not in mEmulatedFunctions, this becomes a no-op. Returns true if the function
-    // call needs to be replaced with an emulated one.
-    bool SetFunctionCalled(TOperator op, const TType &param);
-    bool SetFunctionCalled(TOperator op, const TType &param1, const TType &param2);
-    bool SetFunctionCalled(TOperator op,
-                           const TType &param1,
-                           const TType &param2,
-                           const TType &param3);
-
     class FunctionId
     {
       public:
+        FunctionId();
         FunctionId(TOperator op, const TType *param);
         FunctionId(TOperator op, const TType *param1, const TType *param2);
         FunctionId(TOperator op, const TType *param1, const TType *param2, const TType *param3);
 
+        FunctionId(const FunctionId &) = default;
+        FunctionId &operator=(const FunctionId &) = default;
+
         bool operator==(const FunctionId &other) const;
         bool operator<(const FunctionId &other) const;
 
@@ -85,11 +62,48 @@ class BuiltInFunctionEmulator
         const TType *mParam3;
     };
 
+    // Add functions that need to be emulated.
+    FunctionId addEmulatedFunction(TOperator op,
+                                   const TType *param,
+                                   const char *emulatedFunctionDefinition);
+    FunctionId addEmulatedFunction(TOperator op,
+                                   const TType *param1,
+                                   const TType *param2,
+                                   const char *emulatedFunctionDefinition);
+    FunctionId addEmulatedFunction(TOperator op,
+                                   const TType *param1,
+                                   const TType *param2,
+                                   const TType *param3,
+                                   const char *emulatedFunctionDefinition);
+
+    FunctionId addEmulatedFunctionWithDependency(FunctionId dependency,
+                                                 TOperator op,
+                                                 const TType *param1,
+                                                 const TType *param2,
+                                                 const char *emulatedFunctionDefinition);
+
+  private:
+    class BuiltInFunctionEmulationMarker;
+
+    // Records that a function is called by the shader and might need to be emulated. If the
+    // function is not in mEmulatedFunctions, this becomes a no-op. Returns true if the function
+    // call needs to be replaced with an emulated one.
+    bool SetFunctionCalled(TOperator op, const TType &param);
+    bool SetFunctionCalled(TOperator op, const TType &param1, const TType &param2);
+    bool SetFunctionCalled(TOperator op,
+                           const TType &param1,
+                           const TType &param2,
+                           const TType &param3);
+
     bool SetFunctionCalled(const FunctionId &functionId);
 
     // Map from function id to emulated function definition
     std::map<FunctionId, std::string> mEmulatedFunctions;
 
+    // Map from dependent functions to their dependencies. This structure allows each function to
+    // have at most one dependency.
+    std::map<FunctionId, FunctionId> mFunctionDependencies;
+
     // Called function ids
     std::vector<FunctionId> mFunctions;
 };

src/compiler/translator/BuiltInFunctionEmulatorGLSL.cpp

@@ -75,6 +75,43 @@ void InitBuiltInIsnanFunctionEmulatorForGLSLWorkarounds(BuiltInFunctionEmulator
         "}\n");
 }
 
+void InitBuiltInAtanFunctionEmulatorForGLSLWorkarounds(BuiltInFunctionEmulator *emu)
+{
+    const TType *float1 = TCache::getType(EbtFloat);
+    auto floatFuncId    = emu->addEmulatedFunction(
+        EOpAtan, float1, float1,
+        "webgl_emu_precision float webgl_atan_emu(webgl_emu_precision float y, webgl_emu_precision "
+        "float x)\n"
+        "{\n"
+        "    if (x > 0.0) return atan(y / x);\n"
+        "    else if (x < 0.0 && y >= 0.0) return atan(y / x) + 3.14159265;\n"
+        "    else if (x < 0.0 && y < 0.0) return atan(y / x) - 3.14159265;\n"
+        "    else return 1.57079632 * sign(y);\n"
+        "}\n");
+    for (int dim = 2; dim <= 4; ++dim)
+    {
+        const TType *floatVec = TCache::getType(EbtFloat, static_cast<unsigned char>(dim));
+        std::stringstream ss;
+        ss << "webgl_emu_precision vec" << dim << " webgl_atan_emu(webgl_emu_precision vec" << dim
+           << " y, webgl_emu_precision vec" << dim << " x)\n"
+                                                      "{\n"
+                                                      "    return vec"
+           << dim << "(";
+        for (int i = 0; i < dim; ++i)
+        {
+            ss << "webgl_atan_emu(y[" << i << "], x[" << i << "])";
+            if (i < dim - 1)
+            {
+                ss << ", ";
+            }
+        }
+        ss << ");\n"
+              "}\n";
+        emu->addEmulatedFunctionWithDependency(floatFuncId, EOpAtan, floatVec, floatVec,
+                                               ss.str().c_str());
+    }
+}
+
 // Emulate built-in functions missing from GLSL 1.30 and higher
 void InitBuiltInFunctionEmulatorForGLSLMissingFunctions(BuiltInFunctionEmulator *emu,
                                                         sh::GLenum shaderType,

src/compiler/translator/BuiltInFunctionEmulatorGLSL.h

@@ -24,6 +24,10 @@ void InitBuiltInAbsFunctionEmulatorForGLSLWorkarounds(BuiltInFunctionEmulator *e
 //
 void InitBuiltInIsnanFunctionEmulatorForGLSLWorkarounds(BuiltInFunctionEmulator *emu,
                                                         int targetGLSLVersion);
+//
+// This works around atan(y, x) bug in NVIDIA drivers.
+//
+void InitBuiltInAtanFunctionEmulatorForGLSLWorkarounds(BuiltInFunctionEmulator *emu);
 
 //
 // This function is emulating built-in functions missing from GLSL 1.30 and higher.

src/compiler/translator/TranslatorESSL.cpp

@@ -6,6 +6,7 @@
 
 #include "compiler/translator/TranslatorESSL.h"
 
+#include "compiler/translator/BuiltInFunctionEmulatorGLSL.h"
 #include "compiler/translator/EmulatePrecision.h"
 #include "compiler/translator/RecordConstantPrecision.h"
 #include "compiler/translator/OutputESSL.h"
@@ -19,6 +20,15 @@ TranslatorESSL::TranslatorESSL(sh::GLenum type, ShShaderSpec spec)
 {
 }
 
+void TranslatorESSL::initBuiltInFunctionEmulator(BuiltInFunctionEmulator *emu,
+                                                 ShCompileOptions compileOptions)
+{
+    if (compileOptions & SH_EMULATE_ATAN2_FLOAT_FUNCTION)
+    {
+        InitBuiltInAtanFunctionEmulatorForGLSLWorkarounds(emu);
+    }
+}
+
 void TranslatorESSL::translate(TIntermNode *root, ShCompileOptions compileOptions)
 {
     TInfoSinkBase &sink = getInfoSink().obj;

src/compiler/translator/TranslatorESSL.h

@@ -18,6 +18,9 @@ class TranslatorESSL : public TCompiler
     TranslatorESSL(sh::GLenum type, ShShaderSpec spec);
 
   protected:
+    void initBuiltInFunctionEmulator(BuiltInFunctionEmulator *emu,
+                                     ShCompileOptions compileOptions) override;
+
     void translate(TIntermNode *root, ShCompileOptions compileOptions) override;
     bool shouldFlattenPragmaStdglInvariantAll() override;
 

src/compiler/translator/TranslatorGLSL.cpp

@@ -36,6 +36,11 @@ void TranslatorGLSL::initBuiltInFunctionEmulator(BuiltInFunctionEmulator *emu,
         InitBuiltInIsnanFunctionEmulatorForGLSLWorkarounds(emu, getShaderVersion());
     }
 
+    if (compileOptions & SH_EMULATE_ATAN2_FLOAT_FUNCTION)
+    {
+        InitBuiltInAtanFunctionEmulatorForGLSLWorkarounds(emu);
+    }
+
     int targetGLSLVersion = ShaderOutputTypeToGLSLVersion(getOutputType());
     InitBuiltInFunctionEmulatorForGLSLMissingFunctions(emu, getShaderType(), targetGLSLVersion);
 }

src/libANGLE/renderer/gl/ShaderGL.cpp

@@ -70,6 +70,11 @@ ShCompileOptions ShaderGL::prepareSourceAndReturnOptions(std::stringstream *sour
         options |= SH_EMULATE_ISNAN_FLOAT_FUNCTION;
     }
 
+    if (mWorkarounds.emulateAtan2Float)
+    {
+        options |= SH_EMULATE_ATAN2_FLOAT_FUNCTION;
+    }
+
     if (mWorkarounds.useUnusedBlocksWithStandardOrSharedLayout)
     {
         options |= SH_USE_UNUSED_STANDARD_SHARED_BLOCKS;

src/libANGLE/renderer/gl/WorkaroundsGL.h

@@ -28,7 +28,8 @@ struct WorkaroundsGL
           useUnusedBlocksWithStandardOrSharedLayout(false),
           dontRemoveInvariantForFragmentInput(false),
           removeInvariantAndCentroidForESSL3(false),
-          rewriteFloatUnaryMinusOperator(false)
+          rewriteFloatUnaryMinusOperator(false),
+          emulateAtan2Float(false)
     {
     }
 
@@ -126,6 +127,10 @@ struct WorkaroundsGL
     // replace "-float".
     // Tracking bug: http://crbug.com/308366
     bool rewriteFloatUnaryMinusOperator;
+
+    // On NVIDIA drivers, atan(y, x) may return a wrong answer.
+    // Tracking bug: http://crbug.com/672380
+    bool emulateAtan2Float;
 };
 }
 

src/libANGLE/renderer/gl/renderergl_utils.cpp

@@ -953,6 +953,10 @@ void GenerateWorkarounds(const FunctionsGL *functions, WorkaroundsGL *workaround
     workarounds->removeInvariantAndCentroidForESSL3 =
         functions->isAtMostGL(gl::Version(4, 1)) ||
         (functions->standard == STANDARD_GL_DESKTOP && IsAMD(vendor));
+
+    // TODO(oetuaho): Make this specific to the affected driver versions. Versions that came after
+    // 364 are known to be affected, at least up to 375.
+    workarounds->emulateAtan2Float = IsNvidia(vendor);
 }
 
 }

src/tests/gl_tests/GLSLTest.cpp

@@ -2170,13 +2170,6 @@ TEST_P(GLSLTest, NestedPowStatements)
 // Test that -float calculation is correct.
 TEST_P(GLSLTest_ES3, UnaryMinusOperatorFloat)
 {
-    // TODO(oetuaho@nvidia.com): re-enable once http://crbug.com/672380 is fixed.
-    if ((IsWindows() || IsLinux()) && IsNVIDIA() && IsOpenGL())
-    {
-        std::cout << "Test disabled on this OpenGL configuration." << std::endl;
-        return;
-    }
-
     const std::string &vert =
         "#version 300 es\n"
         "in highp vec4 position;\n"
@@ -2198,6 +2191,31 @@ TEST_P(GLSLTest_ES3, UnaryMinusOperatorFloat)
     EXPECT_PIXEL_COLOR_EQ(0, 0, GLColor::green);
 }
 
+// Test that atan(vec2, vec2) calculation is correct.
+TEST_P(GLSLTest_ES3, AtanVec2)
+{
+    const std::string &vert =
+        "#version 300 es\n"
+        "in highp vec4 position;\n"
+        "void main() {\n"
+        "    gl_Position = position;\n"
+        "}\n";
+    const std::string &frag =
+        "#version 300 es\n"
+        "out highp vec4 o_color;\n"
+        "void main() {\n"
+        "    highp float f = 1.0;\n"
+        "    // atan(tan(0.5), f) should be 0.5.\n"
+        "    highp vec2 v = atan(vec2(tan(0.5)), vec2(f));\n"
+        "    o_color = (abs(v[0] - 0.5) < 0.001 && abs(v[1] - 0.5) < 0.001) ? vec4(0, 1, 0, 1) : "
+        "vec4(1, 0, 0, 1);\n"
+        "}\n";
+
+    ANGLE_GL_PROGRAM(prog, vert, frag);
+    drawQuad(prog.get(), "position", 0.5f);
+    EXPECT_PIXEL_COLOR_EQ(0, 0, GLColor::green);
+}
+
 // Convers a bug with the unary minus operator on signed integer workaround.
 TEST_P(GLSLTest_ES3, UnaryMinusOperatorSignedInt)
 {