[SYSTEMML-2370] Fix IPA/runtime robustness for unbound function outputs This patch fixes issue when compiler function calls with unbound outputs, i.e., calls to a function with outputs without binding them to variables at the calling site. In detail, there were issues during inter-procedural analysis (when extracting size information) and during runtime (when mapping function outputs into the calling context).
Project: http://git-wip-us.apache.org/repos/asf/systemml/repo Commit: http://git-wip-us.apache.org/repos/asf/systemml/commit/15971f79 Tree: http://git-wip-us.apache.org/repos/asf/systemml/tree/15971f79 Diff: http://git-wip-us.apache.org/repos/asf/systemml/diff/15971f79 Branch: refs/heads/master Commit: 15971f7979499b55f0e4befdd99dcabdefb6d865 Parents: f6de9b1 Author: Matthias Boehm <[email protected]> Authored: Wed Jun 6 22:42:53 2018 -0700 Committer: Matthias Boehm <[email protected]> Committed: Wed Jun 6 22:42:53 2018 -0700 ---------------------------------------------------------------------- .../sysml/hops/ipa/InterProceduralAnalysis.java | 44 ++++++------- .../cp/FunctionCallCPInstruction.java | 4 +- .../functions/misc/FunctionPotpourriTest.java | 65 ++++++++++++++++++++ .../functions/misc/FunPotpourriComments.dml | 39 ++++++++++++ .../functions/misc/FunPotpourriNoReturn.dml | 29 +++++++++ .../functions/misc/ZPackageSuite.java | 1 + 6 files changed, 157 insertions(+), 25 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/systemml/blob/15971f79/src/main/java/org/apache/sysml/hops/ipa/InterProceduralAnalysis.java ---------------------------------------------------------------------- diff --git a/src/main/java/org/apache/sysml/hops/ipa/InterProceduralAnalysis.java b/src/main/java/org/apache/sysml/hops/ipa/InterProceduralAnalysis.java index d39b4b6..463a531 100644 --- a/src/main/java/org/apache/sysml/hops/ipa/InterProceduralAnalysis.java +++ b/src/main/java/org/apache/sysml/hops/ipa/InterProceduralAnalysis.java @@ -581,8 +581,11 @@ public class InterProceduralAnalysis try { - for( int i=0; i<foutputOps.size(); i++ ) - { + for( int i=0; i<foutputOps.size(); i++ ) { + //robustness for unbound outputs + if( outputVars.length <= i ) + break; + DataIdentifier di = foutputOps.get(i); String fvarname = di.getName(); //name in function signature String pvarname = outputVars[i]; //name in calling program @@ -600,38 +603,31 @@ public class InterProceduralAnalysis } } // Update or add to the calling program's variable map. - if( di.getDataType()==DataType.MATRIX && tmpVars.keySet().contains(fvarname) ) - { + if( di.getDataType()==DataType.MATRIX && tmpVars.keySet().contains(fvarname) ) { MatrixObject moIn = (MatrixObject) tmpVars.get(fvarname); - - if( !callVars.keySet().contains(pvarname) || overwrite ) //not existing so far - { - MatrixObject moOut = createOutputMatrix(moIn.getNumRows(), moIn.getNumColumns(), moIn.getNnz()); + if( !callVars.keySet().contains(pvarname) || overwrite ) { //not existing so far + MatrixObject moOut = createOutputMatrix(moIn.getNumRows(), moIn.getNumColumns(), moIn.getNnz()); callVars.put(pvarname, moOut); } - else //already existing: take largest - { + else { //already existing: take largest Data dat = callVars.get(pvarname); - if( dat instanceof MatrixObject ) + if( !(dat instanceof MatrixObject) ) + continue; + MatrixObject moOut = (MatrixObject)dat; + MatrixCharacteristics mc = moOut.getMatrixCharacteristics(); + if( OptimizerUtils.estimateSizeExactSparsity(mc.getRows(), mc.getCols(), (mc.getNonZeros()>0)? + OptimizerUtils.getSparsity(mc):1.0) + < OptimizerUtils.estimateSize(moIn.getNumRows(), moIn.getNumColumns()) ) { - MatrixObject moOut = (MatrixObject)dat; - MatrixCharacteristics mc = moOut.getMatrixCharacteristics(); - if( OptimizerUtils.estimateSizeExactSparsity(mc.getRows(), mc.getCols(), (mc.getNonZeros()>0)? - OptimizerUtils.getSparsity(mc):1.0) - < OptimizerUtils.estimateSize(moIn.getNumRows(), moIn.getNumColumns()) ) - { - //update statistics if necessary - mc.setDimension(moIn.getNumRows(), moIn.getNumColumns()); - mc.setNonZeros(moIn.getNnz()); - } + //update statistics if necessary + mc.setDimension(moIn.getNumRows(), moIn.getNumColumns()); + mc.setNonZeros(moIn.getNnz()); } - } } } } - catch( Exception ex ) - { + catch( Exception ex ) { throw new HopsException( "Failed to extract output statistics of function "+fkey+".", ex); } } http://git-wip-us.apache.org/repos/asf/systemml/blob/15971f79/src/main/java/org/apache/sysml/runtime/instructions/cp/FunctionCallCPInstruction.java ---------------------------------------------------------------------- diff --git a/src/main/java/org/apache/sysml/runtime/instructions/cp/FunctionCallCPInstruction.java b/src/main/java/org/apache/sysml/runtime/instructions/cp/FunctionCallCPInstruction.java index dea48bc..1c1ac30 100644 --- a/src/main/java/org/apache/sysml/runtime/instructions/cp/FunctionCallCPInstruction.java +++ b/src/main/java/org/apache/sysml/runtime/instructions/cp/FunctionCallCPInstruction.java @@ -178,7 +178,9 @@ public class FunctionCallCPInstruction extends CPInstruction { ec.unpinVariables(_boundInputNames, pinStatus); // add the updated binding for each return variable to the variables in original symbol table - for (int i=0; i< fpb.getOutputParams().size(); i++){ + // (with robustness for unbound outputs, i.e., function calls without assignment) + int numOutputs = Math.min(_boundOutputNames.size(), fpb.getOutputParams().size()); + for (int i=0; i< numOutputs; i++) { String boundVarName = _boundOutputNames.get(i); Data boundValue = retVars.get(fpb.getOutputParams().get(i).getName()); if (boundValue == null) http://git-wip-us.apache.org/repos/asf/systemml/blob/15971f79/src/test/java/org/apache/sysml/test/integration/functions/misc/FunctionPotpourriTest.java ---------------------------------------------------------------------- diff --git a/src/test/java/org/apache/sysml/test/integration/functions/misc/FunctionPotpourriTest.java b/src/test/java/org/apache/sysml/test/integration/functions/misc/FunctionPotpourriTest.java new file mode 100644 index 0000000..b5bd02f --- /dev/null +++ b/src/test/java/org/apache/sysml/test/integration/functions/misc/FunctionPotpourriTest.java @@ -0,0 +1,65 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.sysml.test.integration.functions.misc; + + +import org.junit.Test; +import org.apache.sysml.api.DMLException; +import org.apache.sysml.test.integration.AutomatedTestBase; +import org.apache.sysml.test.integration.TestConfiguration; +import org.apache.sysml.test.utils.TestUtils; + +public class FunctionPotpourriTest extends AutomatedTestBase +{ + private final static String TEST_NAME1 = "FunPotpourriNoReturn"; + private final static String TEST_NAME2 = "FunPotpourriComments"; + + private final static String TEST_DIR = "functions/misc/"; + private final static String TEST_CLASS_DIR = TEST_DIR + FunctionPotpourriTest.class.getSimpleName() + "/"; + + @Override + public void setUp() { + TestUtils.clearAssertionInformation(); + addTestConfiguration( TEST_NAME1, new TestConfiguration(TEST_CLASS_DIR, TEST_NAME1, new String[] { "R" }) ); + addTestConfiguration( TEST_NAME2, new TestConfiguration(TEST_CLASS_DIR, TEST_NAME2, new String[] { "R" }) ); + } + + @Test + public void testFunctionNoReturn() { + runFunctionTest( TEST_NAME1, false ); + } + + @Test + public void testFunctionComments() { + runFunctionTest( TEST_NAME2, false ); + } + + private void runFunctionTest(String testName, boolean error) { + TestConfiguration config = getTestConfiguration(testName); + loadTestConfiguration(config); + + String HOME = SCRIPT_DIR + TEST_DIR; + fullDMLScriptName = HOME + testName + ".dml"; + programArgs = new String[]{"-explain", "-stats"}; + + //run script and compare output + runTest(true, error, DMLException.class, -1); + } +} http://git-wip-us.apache.org/repos/asf/systemml/blob/15971f79/src/test/scripts/functions/misc/FunPotpourriComments.dml ---------------------------------------------------------------------- diff --git a/src/test/scripts/functions/misc/FunPotpourriComments.dml b/src/test/scripts/functions/misc/FunPotpourriComments.dml new file mode 100644 index 0000000..719b73d --- /dev/null +++ b/src/test/scripts/functions/misc/FunPotpourriComments.dml @@ -0,0 +1,39 @@ +#------------------------------------------------------------- +# +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +# +#------------------------------------------------------------- + + +foo1 = function(matrix[double] X) return(boolean x) { + if( sum(X) > 1 ) { + print("X "+sum(X)); + } # a comment + x = TRUE; +} # a second comment + +X = matrix(7, 10, 10); +foo1(X); +foo2(X); + +foo2 = function(matrix[double] X) return(boolean x) { + if( sum(X) > 1 ) { + print("X "+sum(X)); + } # a comment + x = TRUE; +} # a second comment http://git-wip-us.apache.org/repos/asf/systemml/blob/15971f79/src/test/scripts/functions/misc/FunPotpourriNoReturn.dml ---------------------------------------------------------------------- diff --git a/src/test/scripts/functions/misc/FunPotpourriNoReturn.dml b/src/test/scripts/functions/misc/FunPotpourriNoReturn.dml new file mode 100644 index 0000000..4a061f7 --- /dev/null +++ b/src/test/scripts/functions/misc/FunPotpourriNoReturn.dml @@ -0,0 +1,29 @@ +#------------------------------------------------------------- +# +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +# +#------------------------------------------------------------- + +foo = function(matrix[double] X) return(boolean x) { + if( sum(X) > 1 ) + print("X "+sum(X)); + x = TRUE; +} + +X = matrix(7, 10, 10); +foo(X); http://git-wip-us.apache.org/repos/asf/systemml/blob/15971f79/src/test_suites/java/org/apache/sysml/test/integration/functions/misc/ZPackageSuite.java ---------------------------------------------------------------------- diff --git a/src/test_suites/java/org/apache/sysml/test/integration/functions/misc/ZPackageSuite.java b/src/test_suites/java/org/apache/sysml/test/integration/functions/misc/ZPackageSuite.java index be92a5c..e2c7bf1 100644 --- a/src/test_suites/java/org/apache/sysml/test/integration/functions/misc/ZPackageSuite.java +++ b/src/test_suites/java/org/apache/sysml/test/integration/functions/misc/ZPackageSuite.java @@ -34,6 +34,7 @@ import org.junit.runners.Suite; FunctionInliningTest.class, FunctionNamespaceTest.class, FunctionNotFoundTest.class, + FunctionPotpourriTest.class, FunctionReturnTest.class, IfTest.class, InvalidBuiltinFunctionCallTest.class,
