Fix the OpenCL VLOOKUP to work properly and add it to the trusted subset

Treat an array of null string pointers as no strings for OpenCL. For
some reason, at least in the case of the "Test OpenCL" thing, we got
an mpStringArray that is non-null but where all the elements
(rtl_uString pointers) in it are null. Treat that case as if the
mpStringArray was null.

This makes the tests "Test OpenCL" actually use OpenCL. Maybe it has other
useful effects, too. (But for normal spreadsheet use, the mpStringArray that
gets handled here *is* null when all the cells used by a formula group are
numbers. At least it seemed so in a simple test.)

Add a few more useful (?) SAL_INFO calls.

Some cosmetic changes to make the code look saner.

Return correct value from the OpenCL VLOOKUP implementation. Also,
when the lookup fails, produce the expected N/A error code instead of
a bare NaN.

Don't claim we support strings arguments in the OpenCL VLOOKUP.  The
string support certainly isn't complete or correct anyway. Partially
revert c3383aafa18ef9d03b04b2a4719e71fdfabc14eb.

Add VLOOKUP to the set of opcodes that we trust the OpenCL
implementation for.

(cherry picked from commit 7e91726f4d81f0ab27d79ee231abd666b4999758)
(cherry picked from commit d4310b6cd8a367666cc702c3e47cf12a35407ef7)
(cherry picked from commit 32b35d9d8a21091b987d94fc2ad24d69e9d8a6f3)
(cherry picked from commit 41a6094095a0cd42f24eb996c7ec8c28ab9d6c1a)
(cherry picked from commit 162264450cb62177ea133829d081fecdb02136b5)
(cherry picked from commit 7bb7539c0e34283baeaacf7e4ff0b19287afadc2)

Change-Id: I5402732c9ddcac1efcc25960a5c46175d7a90b4d
Reviewed-on: https://gerrit.libreoffice.org/18022
Reviewed-by: Michael Meeks <michael.meeks@collabora.com>
Reviewed-by: Eike Rathke <erack@redhat.com>
Tested-by: Jan Holesovsky <kendy@collabora.com>

5 files changed, 104 insertions, 94 deletions

diff --git a/officecfg/registry/schema/org/openoffice/Office/Calc.xcs b/officecfg/registry/schema/org/openoffice/Office/Calc.xcs
index bb74d74b6256..c3fa5239a712 100644
--- a/officecfg/registry/schema/org/openoffice/Office/Calc.xcs
+++ b/officecfg/registry/schema/org/openoffice/Office/Calc.xcs
@@ -1373,7 +1373,7 @@
             true, and a formula contains only these operators and
             functions, it might be calculated using OpenCL.</desc>
           </info>
-          <value>+;-;*;/;RAND;SIN;COS;TAN;ATAN;EXP;LN;SQRT;NORMSDIST;NORMSINV;ROUND;POWER;SUMPRODUCT;MIN;MAX;SUM;PRODUCT;AVERAGE;COUNT;VAR;NORMDIST;CORREL;COVAR;PEARSON;SLOPE;SUMIFS</value>
+          <value>+;-;*;/;RAND;SIN;COS;TAN;ATAN;EXP;LN;SQRT;NORMSDIST;NORMSINV;ROUND;POWER;SUMPRODUCT;MIN;MAX;SUM;PRODUCT;AVERAGE;COUNT;VAR;NORMDIST;VLOOKUP;CORREL;COVAR;PEARSON;SLOPE;SUMIFS</value>
         </prop>
         <prop oor:name="OpenCLAutoSelect" oor:type="xs:boolean" oor:nillable="false">
           <!-- UIHints: Tools - Options  Spreadsheet  Formula -->
diff --git a/sc/source/core/opencl/formulagroupcl.cxx b/sc/source/core/opencl/formulagroupcl.cxx
index 368c525a6d3b..e5324cd07e14 100644
--- a/sc/source/core/opencl/formulagroupcl.cxx
+++ b/sc/source/core/opencl/formulagroupcl.cxx
@@ -40,6 +40,7 @@ static const char* publicFunc =
  "#define errIllegalFPOperation 503 // #NUM!\n"
  "#define errNoValue 519 // #VALUE!\n"
  "#define errDivisionByZero 532 // #DIV/0!\n"
+ "#define NOTAVAILABLE 0x7fff // #N/A\n"
  "\n"
  "double CreateDoubleError(ulong nErr)\n"
  "{\n"
@@ -152,6 +153,19 @@ std::string linenumberify(const std::string& s)
 }
 #endif
 
+bool AllStringsAreNull(const rtl_uString* const* pStringArray, size_t nLength)
+{
+    if (pStringArray == nullptr)
+        return true;
+
+    for (size_t i = 0; i < nLength; i++)
+        if (pStringArray[i] != nullptr)
+            return false;
+
+    return true;
+}
+
+
 } // anonymous namespace
 
 /// Map the buffer used by an argument and do necessary argument setting
@@ -2615,8 +2629,10 @@ DynamicKernelSoPArguments::DynamicKernelSoPArguments(const ScCalcConfig& config,
 
                     for (size_t j = 0; j < pDVR->GetArrays().size(); ++j)
                     {
-                        SAL_INFO("sc.opencl", "j=" << j << " mpNumericArray=" << pDVR->GetArrays()[j].mpNumericArray <<
+                        SAL_INFO("sc.opencl", "i=" << i << " j=" << j <<
+                                 " mpNumericArray=" << pDVR->GetArrays()[j].mpNumericArray <<
                                  " mpStringArray=" << pDVR->GetArrays()[j].mpStringArray <<
+                                 " allStringsAreNull=" << (AllStringsAreNull(pDVR->GetArrays()[j].mpStringArray, pDVR->GetArrayLength())?"YES":"NO") <<
                                  " takeNumeric=" << (pCodeGen->takeNumeric()?"YES":"NO") <<
                                  " takeString=" << (pCodeGen->takeString()?"YES":"NO"));
 
@@ -2629,6 +2645,8 @@ DynamicKernelSoPArguments::DynamicKernelSoPArguments(const ScCalcConfig& config,
                                 pDVR->GetArrays()[j].mpStringArray &&
                                 pCodeGen->takeString())
                             {
+                                // Function takes numbers or strings, there are both
+                                SAL_INFO("sc.opencl", "Numbers and strings and that is OK");
                                 mvSubArguments.push_back(
                                     DynamicKernelArgumentRef(
                                         new DynamicKernelMixedSlidingArgument(mCalcConfig,
@@ -2636,16 +2654,22 @@ DynamicKernelSoPArguments::DynamicKernelSoPArguments(const ScCalcConfig& config,
                             }
                             else
                             {
+                                // Not sure I can figure out what case this exactly is;)
+                                SAL_INFO("sc.opencl", "The other case");
                                 mvSubArguments.push_back(
                                     DynamicKernelArgumentRef(VectorRefFactory<VectorRef>(mCalcConfig,
                                             ts, ft->Children[i], mpCodeGen, j)));
                             }
                         }
                         else
+                        {
+                            // Ditto here. This is such crack.
+                            SAL_INFO("sc.opencl", "The outer other case (can't figure out what it exactly means)");
                             mvSubArguments.push_back(
                                 DynamicKernelArgumentRef(VectorRefFactory
                                     <DynamicKernelStringArgument>(mCalcConfig,
                                         ts, ft->Children[i], mpCodeGen, j)));
+                        }
                     }
                 }
                 else if (pChild->GetType() == formula::svSingleVectorRef)
@@ -2653,8 +2677,10 @@ DynamicKernelSoPArguments::DynamicKernelSoPArguments(const ScCalcConfig& config,
                     const formula::SingleVectorRefToken* pSVR =
                         static_cast<const formula::SingleVectorRefToken*>(pChild);
 
-                    SAL_INFO("sc.opencl", "mpNumericArray=" << pSVR->GetArray().mpNumericArray <<
+                    SAL_INFO("sc.opencl", "i=" << i <<
+                             " mpNumericArray=" << pSVR->GetArray().mpNumericArray <<
                              " mpStringArray=" << pSVR->GetArray().mpStringArray <<
+                             " allStringsAreNull=" << (AllStringsAreNull(pSVR->GetArray().mpStringArray, pSVR->GetArrayLength())?"YES":"NO") <<
                              " takeNumeric=" << (pCodeGen->takeNumeric()?"YES":"NO") <<
                              " takeString=" << (pCodeGen->takeString()?"YES":"NO"));
 
@@ -2664,17 +2690,19 @@ DynamicKernelSoPArguments::DynamicKernelSoPArguments(const ScCalcConfig& config,
                         pCodeGen->takeString())
                     {
                         // Function takes numbers or strings, there are both
+                        SAL_INFO("sc.opencl", "Numbers and strings and that is OK");
                         mvSubArguments.push_back(
                             DynamicKernelArgumentRef(new DynamicKernelMixedArgument(mCalcConfig,
                                     ts, ft->Children[i])));
                     }
                     else if (pSVR->GetArray().mpNumericArray &&
                         pCodeGen->takeNumeric() &&
-                        (pSVR->GetArray().mpStringArray == NULL || mCalcConfig.meStringConversion == ScCalcConfig::StringConversion::ZERO))
+                             (AllStringsAreNull(pSVR->GetArray().mpStringArray, pSVR->GetArrayLength()) || mCalcConfig.meStringConversion == ScCalcConfig::StringConversion::ZERO))
                     {
                         // Function takes numbers, and either there
                         // are no strings, or there are strings but
                         // they are to be treated as zero
+                        SAL_INFO("sc.opencl", "Maybe strings even if want numbers but should be treated as zero");
                         mvSubArguments.push_back(
                             DynamicKernelArgumentRef(new VectorRef(mCalcConfig, ts,
                                     ft->Children[i])));
@@ -2686,6 +2714,7 @@ DynamicKernelSoPArguments::DynamicKernelSoPArguments(const ScCalcConfig& config,
                     {
                         // Function takes numbers, and there are only
                         // strings, but they are to be treated as zero
+                        SAL_INFO("sc.opencl", "Only strings even if want numbers but should be treated as zero");
                         mvSubArguments.push_back(
                             DynamicKernelArgumentRef(new VectorRef(mCalcConfig, ts,
                                     ft->Children[i])));
@@ -2693,28 +2722,32 @@ DynamicKernelSoPArguments::DynamicKernelSoPArguments(const ScCalcConfig& config,
                     else if (pSVR->GetArray().mpStringArray &&
                         pCodeGen->takeString())
                     {
-                        // There are strings, and the function takes
-                        // strings.
-
+                        // There are strings, and the function takes strings.
+                        SAL_INFO("sc.opencl", "Strings only");
                         mvSubArguments.push_back(
                             DynamicKernelArgumentRef(new DynamicKernelStringArgument(mCalcConfig,
                                     ts, ft->Children[i])));
                     }
-                    else if (pSVR->GetArray().mpStringArray == NULL &&
+                    else if (AllStringsAreNull(pSVR->GetArray().mpStringArray, pSVR->GetArrayLength()) &&
                         pSVR->GetArray().mpNumericArray == NULL)
                     {
                         // There are only empty cells. Push as an
                         // array of NANs
+                        SAL_INFO("sc.opencl", "Only empty cells");
                         mvSubArguments.push_back(
                             DynamicKernelArgumentRef(new VectorRef(mCalcConfig, ts,
                                     ft->Children[i])));
                     }
                     else
+                    {
+                        SAL_INFO("sc.opencl", "Fallback case, rejecting for OpenCL");
                         throw UnhandledToken(pChild,
                             "Got unhandled case here", __FILE__, __LINE__);
+                    }
                 }
                 else if (pChild->GetType() == formula::svDouble)
                 {
+                    SAL_INFO("sc.opencl", "Constant number (?) case");
                     mvSubArguments.push_back(
                         DynamicKernelArgumentRef(new DynamicKernelConstantArgument(mCalcConfig, ts,
                                 ft->Children[i])));
@@ -2722,12 +2755,14 @@ DynamicKernelSoPArguments::DynamicKernelSoPArguments(const ScCalcConfig& config,
                 else if (pChild->GetType() == formula::svString
                     && pCodeGen->takeString())
                 {
+                    SAL_INFO("sc.opencl", "Constant string (?) case");
                     mvSubArguments.push_back(
                         DynamicKernelArgumentRef(new ConstStringArgument(mCalcConfig, ts,
                                 ft->Children[i])));
                 }
                 else
                 {
+                    SAL_INFO("sc.opencl", "Fallback case, rejecting for OpenCL");
                     throw UnhandledToken(pChild, ("unhandled operand " + StackVarEnumToString(pChild->GetType()) + " for ocPush").c_str());
                 }
                 break;
diff --git a/sc/source/core/opencl/op_spreadsheet.cxx b/sc/source/core/opencl/op_spreadsheet.cxx
index d9809ef9bf63..10ce77e14e83 100644
--- a/sc/source/core/opencl/op_spreadsheet.cxx
+++ b/sc/source/core/opencl/op_spreadsheet.cxx
@@ -35,25 +35,27 @@ void OpVLookup::GenSlidingWindowFunction(std::stringstream &ss,
     }
     ss << ")\n    {\n";
     ss << "    int gid0=get_global_id(0);\n";
-    ss << "    double tmp = NAN;\n";
+    ss << "    double tmp = CreateDoubleError(NOTAVAILABLE);\n";
     ss << "    double intermediate = DBL_MAX;\n";
     ss << "    int singleIndex = gid0;\n";
     ss << "    int rowNum = -1;\n";
+
     GenTmpVariables(ss,vSubArguments);
     int arg=0;
     CheckSubArgumentIsNan(ss,vSubArguments,arg++);
     int secondParaWidth = 1;
-    if(vSubArguments[1]->GetFormulaToken()->GetType() ==
-    formula::svDoubleVectorRef)
+
+    if (vSubArguments[1]->GetFormulaToken()->GetType() == formula::svDoubleVectorRef)
     {
         FormulaToken *tmpCur = vSubArguments[1]->GetFormulaToken();
-        const formula::DoubleVectorRefToken*pCurDVR= static_cast<const
-            formula::DoubleVectorRefToken *>(tmpCur);
+        const formula::DoubleVectorRefToken*pCurDVR = static_cast<const formula::DoubleVectorRefToken *>(tmpCur);
         secondParaWidth = pCurDVR->GetArrays().size();
     }
-    arg+=secondParaWidth;
+
+    arg += secondParaWidth;
     CheckSubArgumentIsNan(ss,vSubArguments,arg++);
-    if(vSubArguments.size() == (unsigned int)(3+(secondParaWidth-1)))
+
+    if (vSubArguments.size() == (unsigned int)(3+(secondParaWidth-1)))
     {
         ss << "    double tmp";
         ss << 3+(secondParaWidth-1);
@@ -64,53 +66,57 @@ void OpVLookup::GenSlidingWindowFunction(std::stringstream &ss,
         CheckSubArgumentIsNan(ss,vSubArguments,arg++);
     }
 
-    if(vSubArguments[1]->GetFormulaToken()->GetType() ==
-    formula::svDoubleVectorRef)
+    if (vSubArguments[1]->GetFormulaToken()->GetType() == formula::svDoubleVectorRef)
     {
         FormulaToken *tmpCur = vSubArguments[1]->GetFormulaToken();
-        const formula::DoubleVectorRefToken*pCurDVR= static_cast<const
-            formula::DoubleVectorRefToken *>(tmpCur);
-        size_t nCurWindowSize = pCurDVR->GetArrayLength() <
-        pCurDVR->GetRefRowSize() ? pCurDVR->GetArrayLength():
-        pCurDVR->GetRefRowSize() ;
+        const formula::DoubleVectorRefToken*pCurDVR = static_cast<const formula::DoubleVectorRefToken *>(tmpCur);
+        size_t nCurWindowSize = pCurDVR->GetArrayLength() < pCurDVR->GetRefRowSize() ? pCurDVR->GetArrayLength() : pCurDVR->GetRefRowSize() ;
         int unrollSize = 8;
         ss << "    int loop;\n";
-        if (!pCurDVR->IsStartFixed() && pCurDVR->IsEndFixed()) {
+        if (!pCurDVR->IsStartFixed() && pCurDVR->IsEndFixed())
+        {
             ss << "    loop = ("<<nCurWindowSize<<" - gid0)/";
             ss << unrollSize<<";\n";
 
-        } else if (pCurDVR->IsStartFixed() && !pCurDVR->IsEndFixed()) {
+        }
+        else if (pCurDVR->IsStartFixed() && !pCurDVR->IsEndFixed())
+        {
             ss << "    loop = ("<<nCurWindowSize<<" + gid0)/";
             ss << unrollSize<<";\n";
 
-        } else {
+        }
+        else
+        {
             ss << "    loop = "<<nCurWindowSize<<"/"<< unrollSize<<";\n";
         }
 
-        for(int i=0;i<secondParaWidth;i++)
+        for (int i = 0; i < secondParaWidth; i++)
         {
-
             ss << "    for ( int j = 0;j< loop; j++)\n";
             ss << "    {\n";
             ss << "        int i = ";
-            if (!pCurDVR->IsStartFixed()&& pCurDVR->IsEndFixed()) {
-               ss << "gid0 + j * "<< unrollSize <<";\n";
-            }else {
-               ss << "j * "<< unrollSize <<";\n";
+            if (!pCurDVR->IsStartFixed()&& pCurDVR->IsEndFixed())
+            {
+                ss << "gid0 + j * "<< unrollSize <<";\n";
             }
-            if(!pCurDVR->IsStartFixed() && !pCurDVR->IsEndFixed())
+            else
             {
-               ss << "        int doubleIndex = i+gid0;\n";
-            }else
+                ss << "j * "<< unrollSize <<";\n";
+            }
+            if (!pCurDVR->IsStartFixed() && !pCurDVR->IsEndFixed())
             {
-               ss << "        int doubleIndex = i;\n";
+                ss << "        int doubleIndex = i+gid0;\n";
+            }
+            else
+            {
+                ss << "        int doubleIndex = i;\n";
             }
             ss << "        if(tmp";
             ss << 3+(secondParaWidth-1);
             ss << " == 1)\n";
             ss << "        {\n";
 
-            for(int j =0;j < unrollSize;j++)
+            for (int j = 0;j < unrollSize; j++)
             {
                 CheckSubArgumentIsNan(ss,vSubArguments,1+i);
 
@@ -131,7 +137,7 @@ void OpVLookup::GenSlidingWindowFunction(std::stringstream &ss,
 
             ss << "        }else\n";
             ss << "        {\n";
-            for(int j =0;j < unrollSize;j++)
+            for (int j = 0; j < unrollSize; j++)
             {
                 CheckSubArgumentIsNan(ss,vSubArguments,1+i);
 
@@ -149,55 +155,41 @@ void OpVLookup::GenSlidingWindowFunction(std::stringstream &ss,
             ss << "    }\n";
             ss << "    if(rowNum!=-1)\n";
             ss << "    {\n";
-            for(int j=0;j< secondParaWidth; j++)
+            for (int j = 0; j < secondParaWidth; j++)
             {
-
                 ss << "        if(tmp";
                 ss << 2+(secondParaWidth-1);
                 ss << " == ";
                 ss << j+1;
                 ss << ")\n";
-                if( !(vSubArguments[1+j]->IsMixedArgument()))
-                {
-                    ss << "        {\n";
-                    ss << "            tmp = ";
-                    vSubArguments[1+j]->GenDeclRef(ss);
-                    ss << "[rowNum];\n";
-                    ss << "        }\n";
-
-                }
-                else
-                {
-                    ss << "        {\n";
-                    ss << "            tmp = isNan(";
-                    vSubArguments[1+j]->GenNumDeclRef(ss);
-                    ss << "[rowNum])?";
-                    vSubArguments[1+j]->GenNumDeclRef(ss);
-                    ss << "[rowNum]:";
-                    vSubArguments[1+j]->GenStringDeclRef(ss);
-                    ss << "[rowNum];\n";
-                    ss << "        }\n";
-
-                }
+                ss << "            tmp = ";
+                vSubArguments[1+j]->GenDeclRef(ss);
+                ss << "[rowNum];\n";
             }
             ss << "        return tmp;\n";
             ss << "    }\n";
             ss << "    for (int i = ";
-            if (!pCurDVR->IsStartFixed() && pCurDVR->IsEndFixed()) {
-               ss << "gid0 + loop *"<<unrollSize<<"; i < ";
-               ss << nCurWindowSize <<"; i++)\n";
-            } else if (pCurDVR->IsStartFixed() && !pCurDVR->IsEndFixed()) {
-               ss << "0 + loop *"<<unrollSize<<"; i < gid0+";
-               ss << nCurWindowSize <<"; i++)\n";
-            } else {
-               ss << "0 + loop *"<<unrollSize<<"; i < ";
-               ss << nCurWindowSize <<"; i++)\n";
+            if (!pCurDVR->IsStartFixed() && pCurDVR->IsEndFixed())
+            {
+                ss << "gid0 + loop *"<<unrollSize<<"; i < ";
+                ss << nCurWindowSize <<"; i++)\n";
+            }
+            else if (pCurDVR->IsStartFixed() && !pCurDVR->IsEndFixed())
+            {
+                ss << "0 + loop *"<<unrollSize<<"; i < gid0+";
+                ss << nCurWindowSize <<"; i++)\n";
+            }
+            else
+            {
+                ss << "0 + loop *"<<unrollSize<<"; i < ";
+                ss << nCurWindowSize <<"; i++)\n";
             }
             ss << "    {\n";
-            if(!pCurDVR->IsStartFixed() && !pCurDVR->IsEndFixed())
+            if (!pCurDVR->IsStartFixed() && !pCurDVR->IsEndFixed())
             {
                ss << "        int doubleIndex = i+gid0;\n";
-            }else
+            }
+            else
             {
                ss << "        int doubleIndex = i;\n";
             }
@@ -232,38 +224,21 @@ void OpVLookup::GenSlidingWindowFunction(std::stringstream &ss,
             ss << "    if(rowNum!=-1)\n";
             ss << "    {\n";
 
-            for(int j=0;j< secondParaWidth; j++)
+            for (int j = 0; j < secondParaWidth; j++)
             {
-
                 ss << "        if(tmp";
                 ss << 2+(secondParaWidth-1);
                 ss << " == ";
                 ss << j+1;
                 ss << ")\n";
-                ///Add MixedArguments for string support in Vlookup.
-                if( !(vSubArguments[1+j]->IsMixedArgument()))
-                {
-                    ss << "            tmp = ";
-                    vSubArguments[1+j]->GenDeclRef(ss);
-                    ss << "[rowNum];\n";
-                }
-                else
-                {
-                    ss << "            tmp = isNan(";
-                    vSubArguments[1+j]->GenNumDeclRef(ss);
-                    ss << "[rowNum])?";
-                    vSubArguments[1+j]->GenNumDeclRef(ss);
-                    ss << "[rowNum]:";
-                    vSubArguments[1+j]->GenStringDeclRef(ss);
-                    ss << "[rowNum];\n";
-                }
-
+                ss << "            tmp = ";
+                vSubArguments[1+j]->GenDeclRef(ss);
+                ss << "[rowNum];\n";
             }
             ss << "        return tmp;\n";
             ss << "    }\n";
 
         }
-
     }
     else
     {
diff --git a/sc/source/core/opencl/op_spreadsheet.hxx b/sc/source/core/opencl/op_spreadsheet.hxx
index e787bda0a4cd..190ab1dca135 100644
--- a/sc/source/core/opencl/op_spreadsheet.hxx
+++ b/sc/source/core/opencl/op_spreadsheet.hxx
@@ -20,7 +20,6 @@ public:
     virtual void GenSlidingWindowFunction(std::stringstream &ss,
             const std::string &sSymName, SubArguments &vSubArguments) SAL_OVERRIDE;
     virtual std::string BinFuncName() const SAL_OVERRIDE { return "VLookup"; }
-    virtual bool takeString() const SAL_OVERRIDE { return true; }
 };
 
 }}
diff --git a/sc/source/core/tool/calcconfig.cxx b/sc/source/core/tool/calcconfig.cxx
index cc52ad7b3de7..868ddd1b3e8b 100644
--- a/sc/source/core/tool/calcconfig.cxx
+++ b/sc/source/core/tool/calcconfig.cxx
@@ -67,6 +67,7 @@ void ScCalcConfig::setOpenCLConfigToDefault()
     maOpenCLSubsetOpCodes.insert(ocCount);
     maOpenCLSubsetOpCodes.insert(ocVar);
     maOpenCLSubsetOpCodes.insert(ocNormDist);
+    maOpenCLSubsetOpCodes.insert(ocVLookup);
     maOpenCLSubsetOpCodes.insert(ocCorrel);
     maOpenCLSubsetOpCodes.insert(ocCovar);
     maOpenCLSubsetOpCodes.insert(ocPearson);