tdf#94924: Fix several OpenCL problems

Squash several fixes and a unit test into one:

+ Fix handling of empty cells in OpenCL subtraction

We get correct result by simplifying the code;)

No need to have the outer "if (gid0 < X)" test around the calculation
code generated by Reduction::GenSlidingWindowFunction(). The lhs and
rhs check the gid0 range themselves and that leads to the desired
result for subtraction.

While fixing this I noticed that the handling of empty cells in
division is also wrong. Will fix in another commit.

+ Fix handling of empty cells in OpenCL division

Not sure why the code from f5e7207053b857b6903a0ab9c161bed9ad7bcee9
did not produce correct results any longer. Anyway, now OpenCL
division works right in case of empty or zero cells.

Clearly I need to add unit tests to make sure this stuff keeps
working. In later commits.

+ Return correct #DIV/0! error from AVERAGE in the OpenCL case

+ Return correct result 0 from OpenCL MIN and MAX when all args empty

Used the same style as existing code, added a new virtual isMinOrMax()
and add some special casing in Reduction::GenSlidingWindowFunction(),
and fsim_count() and fmax_count() functions that count how many
non-NaN numbers we actually see. As such, I am not sure at all that
this is an ideal way to do this, but will have to do for now.

+ Add a more systematic OpenCL unit test

Avoid the horrible convention of hard-coding in a C++ unit test code
addresses of data in the spreadsheet document being tested. Instead,
mark the expected (= as calculated by Excel) and calculated (by
LibreOffice) formula results, rectangular blocks of data, so that the
C++ code can easily find it, and then compare. This is much more
flexible. No need to edit hardoded row and column numbers in the C++
code when adding more test data.

The systematic.xls file has documentation on how to maintain it.

Change-Id: Ib6e613251bd4a4c96525869611781624cf472ad2
Signed-off-by: Michael Meeks <michael.meeks@collabora.com>
Reviewed-on: https://gerrit.libreoffice.org/19413
Reviewed-by: Tor Lillqvist <tml@collabora.com>
Tested-by: Tor Lillqvist <tml@collabora.com>
Reviewed-by: Eike Rathke <erack@redhat.com>
Reviewed-by: Caolán McNamara <caolanm@redhat.com>
Tested-by: Caolán McNamara <caolanm@redhat.com>

3 files changed, 129 insertions, 28 deletions

diff --git a/sc/qa/unit/data/xls/systematic.xls b/sc/qa/unit/data/xls/systematic.xls
new file mode 100755
index 000000000000..b3427b51a956
--- /dev/null
+++ b/sc/qa/unit/data/xls/systematic.xls
diff --git a/sc/qa/unit/opencl-test.cxx b/sc/qa/unit/opencl-test.cxx
index 17b0e1198ecf..7848627cb8c6 100644
--- a/sc/qa/unit/opencl-test.cxx
+++ b/sc/qa/unit/opencl-test.cxx
@@ -66,6 +66,7 @@ public:
     virtual bool load( const OUString &rFilter, const OUString &rURL,
             const OUString &rUserData, SfxFilterFlags nFilterFlags,
             SotClipboardFormatId nClipboardID, unsigned int nFilterVersion) SAL_OVERRIDE;
+    void testSystematic();
     void testSharedFormulaXLS();
 #if 0
     void testSharedFormulaXLSGroundWater();
@@ -299,6 +300,7 @@ public:
     void testFinancialMDurationFormula1();
 
     CPPUNIT_TEST_SUITE(ScOpenCLTest);
+    CPPUNIT_TEST(testSystematic);
     CPPUNIT_TEST(testSharedFormulaXLS);
     CPPUNIT_TEST(testFinacialFormula);
     CPPUNIT_TEST(testStatisticalFormulaFisher);
@@ -702,6 +704,98 @@ void ScOpenCLTest::testSharedFormulaXLSGroundWater()
 }
 #endif
 
+void ScOpenCLTest::testSystematic()
+{
+    if(!initTestEnv("systematic.", XLS, false))
+        return;
+
+    ScDocument& rDoc = xDocSh->GetDocument();
+    rDoc.CalcAll();
+
+    int nAVertBegin(0), nAVertEnd(0), nBVertBegin(0), nBVertEnd(0);
+    int nAHorEnd(0), nBHorEnd(0);
+
+    int nRow, nCol;
+    for (nRow = 0; nRow < 1000; ++nRow)
+    {
+        if (rDoc.GetString(ScAddress(0, nRow, 0)) == "a")
+        {
+            nAVertBegin = nRow + 1;
+
+            for (nCol = 0; nCol < 1000; ++nCol)
+            {
+                if (rDoc.GetString(ScAddress(nCol, nRow, 0)) != "a")
+                {
+                    nAHorEnd = nCol;
+                    break;
+                }
+            }
+            break;
+        }
+    }
+    for (; nRow < 1000; ++nRow)
+    {
+        if (rDoc.GetString(ScAddress(0, nRow, 0)) != "a")
+        {
+            nAVertEnd = nRow;
+            break;
+        }
+    }
+
+    for (; nRow < 1000; ++nRow)
+    {
+        if (rDoc.GetString(ScAddress(0, nRow, 0)) == "b")
+        {
+            nBVertBegin = nRow + 1;
+
+            for (nCol = 0; nCol < 1000; ++nCol)
+            {
+                if (rDoc.GetString(ScAddress(nCol, nRow, 0)) != "b")
+                {
+                    nBHorEnd = nCol;
+                    break;
+                }
+            }
+            break;
+        }
+    }
+    for (; nRow < 1000; ++nRow)
+    {
+        if (rDoc.GetString(ScAddress(0, nRow, 0)) != "b")
+        {
+            nBVertEnd = nRow;
+            break;
+        }
+    }
+
+    assert(nAVertBegin != 0);
+    assert(nBVertBegin != 0);
+    assert(nAVertEnd > nAVertBegin + 100);
+    assert(nBVertEnd > nBVertBegin + 100);
+    assert((nAVertEnd-nAVertBegin) == (nBVertEnd-nBVertBegin));
+    assert(nAHorEnd > 10);
+    assert(nBHorEnd > 10);
+    assert(nAHorEnd == nBHorEnd);
+
+    for (SCROW i = nAVertBegin; i < nAVertEnd; ++i)
+    {
+        for (int j = 1; j < nAHorEnd; ++j)
+        {
+            double fLibre = rDoc.GetValue(ScAddress(j, i, 0));
+            double fExcel = rDoc.GetValue(ScAddress(j, nBVertBegin + (i - nAVertBegin), 0));
+
+            const OString sFailedMessage =
+                OString(static_cast<sal_Char>('A'+j)) +
+                OString::number(i+1) +
+                "!=" +
+                OString(static_cast<sal_Char>('A'+j)) +
+                OString::number(nBVertBegin+(i-nAVertBegin)+1);
+            CPPUNIT_ASSERT_DOUBLES_EQUAL_MESSAGE(sFailedMessage.getStr(), fExcel, fLibre, 1e-10);
+        }
+    }
+}
+
+
 void ScOpenCLTest::testSharedFormulaXLS()
 {
     if(!initTestEnv("sum_ex.", XLS, false))
diff --git a/sc/source/core/opencl/formulagroupcl.cxx b/sc/source/core/opencl/formulagroupcl.cxx
index e5324cd07e14..2667f5d67e96 100644
--- a/sc/source/core/opencl/formulagroupcl.cxx
+++ b/sc/source/core/opencl/formulagroupcl.cxx
@@ -64,6 +64,18 @@ static const char* publicFunc =
  "    (*p) += t?0:1;\n"
  "    return t?b:a+b;\n"
  "}\n"
+ "double fmin_count(double a, double b, __private int *p) {\n"
+ "    double result = fmin(a, b);\n"
+ "    bool t = isnan(result);\n"
+ "    (*p) += t?0:1;\n"
+ "    return result;\n"
+ "}\n"
+ "double fmax_count(double a, double b, __private int *p) {\n"
+ "    double result = fmax(a, b);\n"
+ "    bool t = isnan(result);\n"
+ "    (*p) += t?0:1;\n"
+ "    return result;\n"
+ "}\n"
  "double fsum(double a, double b) { return isNan(a)?b:a+b; }\n"
  "double legalize(double a, double b) { return isNan(a)?b:a;}\n"
  "double fsub(double a, double b) { return a-b; }\n"
@@ -1698,7 +1710,7 @@ public:
         ss << ") {\n";
         ss << "double tmp = " << GetBottom() << ";\n";
         ss << "int gid0 = get_global_id(0);\n";
-        if (isAverage())
+        if (isAverage() || isMinOrMax())
             ss << "int nCount = 0;\n";
         ss << "double tmpBottom;\n";
         unsigned i = vSubArguments.size();
@@ -1734,13 +1746,8 @@ public:
                 assert(pCur);
                 assert(pCur->GetType() != formula::svDoubleVectorRef);
 
-                if (pCur->GetType() == formula::svSingleVectorRef)
-                {
-                    const formula::SingleVectorRefToken* pSVR =
-                        static_cast<const formula::SingleVectorRefToken*>(pCur);
-                    ss << "if (gid0 < " << pSVR->GetArrayLength() << "){\n";
-                }
-                else if (pCur->GetType() == formula::svDouble)
+                if (pCur->GetType() == formula::svSingleVectorRef ||
+                    pCur->GetType() == formula::svDouble)
                 {
                     ss << "{\n";
                 }
@@ -1771,13 +1778,6 @@ public:
                 ss << ";\n";
                 ss << "    }\n";
                 ss << "}\n";
-                if (vSubArguments[i]->GetFormulaToken()->GetType() ==
-                        formula::svSingleVectorRef && ZeroReturnZero())
-                {
-                    ss << "else{\n";
-                    ss << "        return 0;\n";
-                    ss << "    }\n";
-                }
             }
             else
             {
@@ -1786,12 +1786,21 @@ public:
                 ss << ";\n";
             }
         }
+        if (isAverage())
+            ss <<
+                "if (nCount==0)\n"
+                "    return CreateDoubleError(errDivisionByZero);\n";
+        else if (isMinOrMax())
+            ss <<
+                "if (nCount==0)\n"
+                "    return 0;\n";
         ss << "return tmp";
         if (isAverage())
             ss << "*pow((double)nCount,-1.0)";
         ss << ";\n}";
     }
     virtual bool isAverage() const { return false; }
+    virtual bool isMinOrMax() const { return false; }
     virtual bool takeString() const SAL_OVERRIDE { return false; }
     virtual bool takeNumeric() const SAL_OVERRIDE { return true; }
 };
@@ -2134,7 +2143,7 @@ public:
         ss << "fsum_count(" << lhs << "," << rhs << ", &nCount)";
         return ss.str();
     }
-    virtual std::string BinFuncName() const SAL_OVERRIDE { return "fsum"; }
+    virtual std::string BinFuncName() const SAL_OVERRIDE { return "average"; }
     virtual bool isAverage() const SAL_OVERRIDE { return true; }
 };
 
@@ -2184,20 +2193,16 @@ public:
         {
             ss <<
                 "if (isnan(" << vSubArguments[argno]->GenSlidingWindowDeclRef() << ")) {\n"
-                "    if (GetDoubleErrorValue(" << vSubArguments[argno]->GenSlidingWindowDeclRef() << ") == errNoValue)\n"
-                "        return CreateDoubleError(errDivisionByZero);\n"
+                "    return CreateDoubleError(errDivisionByZero);\n"
                 "}\n";
             return true;
         }
         else if (argno == 0)
         {
             ss <<
-                "if (isnan(" << vSubArguments[argno]->GenSlidingWindowDeclRef() << ")) {\n"
-                "    if (GetDoubleErrorValue(" << vSubArguments[argno]->GenSlidingWindowDeclRef() << ") == errNoValue) {\n"
-                "        if (" << vSubArguments[1]->GenSlidingWindowDeclRef() << " == 0)\n"
-                "            return CreateDoubleError(errDivisionByZero);\n"
-                "        return 0;\n"
-                "    }\n"
+                "if (isnan(" << vSubArguments[argno]->GenSlidingWindowDeclRef() << ") &&\n"
+                "    !(isnan(" << vSubArguments[1]->GenSlidingWindowDeclRef() << ") || " << vSubArguments[1]->GenSlidingWindowDeclRef() << " == 0)) {\n"
+                "    return 0;\n"
                 "}\n";
         }
         return false;
@@ -2210,12 +2215,13 @@ class OpMin : public Reduction
 public:
     OpMin( int nResultSize ) : Reduction(nResultSize) {}
 
-    virtual std::string GetBottom() SAL_OVERRIDE { return "MAXFLOAT"; }
+    virtual std::string GetBottom() SAL_OVERRIDE { return "NAN"; }
     virtual std::string Gen2( const std::string& lhs, const std::string& rhs ) const SAL_OVERRIDE
     {
-        return "fmin(" + lhs + "," + rhs + ")";
+        return "fmin_count(" + lhs + "," + rhs + ", &nCount)";
     }
     virtual std::string BinFuncName() const SAL_OVERRIDE { return "min"; }
+    virtual bool isMinOrMax() const SAL_OVERRIDE { return true; }
 };
 
 class OpMax : public Reduction
@@ -2223,12 +2229,13 @@ class OpMax : public Reduction
 public:
     OpMax( int nResultSize ) : Reduction(nResultSize) {}
 
-    virtual std::string GetBottom() SAL_OVERRIDE { return "-MAXFLOAT"; }
+    virtual std::string GetBottom() SAL_OVERRIDE { return "NAN"; }
     virtual std::string Gen2( const std::string& lhs, const std::string& rhs ) const SAL_OVERRIDE
     {
-        return "fmax(" + lhs + "," + rhs + ")";
+        return "fmax_count(" + lhs + "," + rhs + ", &nCount)";
     }
     virtual std::string BinFuncName() const SAL_OVERRIDE { return "max"; }
+    virtual bool isMinOrMax() const SAL_OVERRIDE { return true; }
 };
 
 class OpSumProduct : public SumOfProduct