From 2eac5c406f10f10c943d0c128dfe6a5db3972e1a Mon Sep 17 00:00:00 2001 From: Tor Lillqvist Date: Wed, 14 Oct 2015 21:14:04 +0300 Subject: tdf#94924: Fix several OpenCL problems MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Reviewed-on: https://gerrit.libreoffice.org/19413 Reviewed-by: Tor Lillqvist Tested-by: Tor Lillqvist Reviewed-by: Eike Rathke Reviewed-by: Caolán McNamara Tested-by: Caolán McNamara --- sc/qa/unit/data/xls/systematic.xls | Bin 0 -> 77824 bytes sc/qa/unit/opencl-test.cxx | 94 +++++++++++++++++++++++++++++++ sc/source/core/opencl/formulagroupcl.cxx | 63 ++++++++++++--------- 3 files changed, 129 insertions(+), 28 deletions(-) create mode 100755 sc/qa/unit/data/xls/systematic.xls 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 Binary files /dev/null and b/sc/qa/unit/data/xls/systematic.xls differ 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('A'+j)) + + OString::number(i+1) + + "!=" + + OString(static_cast('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(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 -- cgit v1.2.3