[SYSTEMML-2340] Fix indexing hop size reset on negative reconciliation This patch fixes a loop size propagation issue that combined with other rewrites can cause crashes and incorrect results. Specifically, the right indexing hop did not properly allow for size resets as required for correct reset passes on negative reconciliation after loops. Furthermore, this patch also adds various tests to ensure this behavior is preserved for future modifications.
Project: http://git-wip-us.apache.org/repos/asf/systemml/repo Commit: http://git-wip-us.apache.org/repos/asf/systemml/commit/b85aea34 Tree: http://git-wip-us.apache.org/repos/asf/systemml/tree/b85aea34 Diff: http://git-wip-us.apache.org/repos/asf/systemml/diff/b85aea34 Branch: refs/heads/master Commit: b85aea34813e26b48a81f9c2ccb21475f7824af9 Parents: 040c100 Author: Matthias Boehm <[email protected]> Authored: Tue May 22 22:16:36 2018 -0700 Committer: Matthias Boehm <[email protected]> Committed: Tue May 22 22:16:36 2018 -0700 ---------------------------------------------------------------------- .../java/org/apache/sysml/hops/IndexingOp.java | 8 ++ .../misc/SizePropagationRBindTest.java | 83 -------------- .../functions/misc/SizePropagationTest.java | 107 +++++++++++++++++++ .../recompile/FunctionRecompileTest.java | 41 +++---- .../functions/misc/SizePropagationLoopIx1.dml | 31 ++++++ .../functions/misc/SizePropagationLoopIx2.dml | 31 ++++++ .../functions/misc/ZPackageSuite.java | 2 +- 7 files changed, 193 insertions(+), 110 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/systemml/blob/b85aea34/src/main/java/org/apache/sysml/hops/IndexingOp.java ---------------------------------------------------------------------- diff --git a/src/main/java/org/apache/sysml/hops/IndexingOp.java b/src/main/java/org/apache/sysml/hops/IndexingOp.java index 2f52f29..8602dee 100644 --- a/src/main/java/org/apache/sysml/hops/IndexingOp.java +++ b/src/main/java/org/apache/sysml/hops/IndexingOp.java @@ -444,6 +444,10 @@ public class IndexingOp extends Hop else if( isBlockIndexingExpression(input2, input3) ) { setDim1(getBlockIndexingExpressionSize(input2, input3)); } + else { + //for reset (e.g., on reconcile after loops) + setDim1(-1); + } if( _colLowerEqualsUpper ) //COLS setDim2(1); @@ -458,6 +462,10 @@ public class IndexingOp extends Hop else if( isBlockIndexingExpression(input4, input5) ) { setDim2(getBlockIndexingExpressionSize(input4, input5)); } + else { + //for reset (e.g., on reconcile after loops) + setDim2(-1); + } } @Override http://git-wip-us.apache.org/repos/asf/systemml/blob/b85aea34/src/test/java/org/apache/sysml/test/integration/functions/misc/SizePropagationRBindTest.java ---------------------------------------------------------------------- diff --git a/src/test/java/org/apache/sysml/test/integration/functions/misc/SizePropagationRBindTest.java b/src/test/java/org/apache/sysml/test/integration/functions/misc/SizePropagationRBindTest.java deleted file mode 100644 index bde8fc7..0000000 --- a/src/test/java/org/apache/sysml/test/integration/functions/misc/SizePropagationRBindTest.java +++ /dev/null @@ -1,83 +0,0 @@ -/* - * 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.junit.Assert; - -import java.util.HashMap; - -import org.apache.sysml.api.DMLScript.RUNTIME_PLATFORM; -import org.apache.sysml.hops.OptimizerUtils; -import org.apache.sysml.runtime.matrix.data.MatrixValue.CellIndex; -import org.apache.sysml.test.integration.AutomatedTestBase; -import org.apache.sysml.test.integration.TestConfiguration; -import org.apache.sysml.test.utils.TestUtils; - -public class SizePropagationRBindTest extends AutomatedTestBase -{ - private static final String TEST_NAME1 = "SizePropagationRBind"; - - private static final String TEST_DIR = "functions/misc/"; - private static final String TEST_CLASS_DIR = TEST_DIR + SizePropagationRBindTest.class.getSimpleName() + "/"; - - private static final int N = 100; - - @Override - public void setUp() { - TestUtils.clearAssertionInformation(); - addTestConfiguration( TEST_NAME1, new TestConfiguration(TEST_CLASS_DIR, TEST_NAME1, new String[] { "R" }) ); - } - - @Test - public void testSizePropagationRBindNoRewrites() { - testSizePropagationRBind( TEST_NAME1, false ); - } - - @Test - public void testSizePropagationRBindRewrites() { - testSizePropagationRBind( TEST_NAME1, true ); - } - - private void testSizePropagationRBind( String testname, boolean rewrites ) { - boolean oldFlag = OptimizerUtils.ALLOW_ALGEBRAIC_SIMPLIFICATION; - RUNTIME_PLATFORM oldPlatform = rtplatform; - - try { - TestConfiguration config = getTestConfiguration(testname); - loadTestConfiguration(config); - - String HOME = SCRIPT_DIR + TEST_DIR; - fullDMLScriptName = HOME + testname + ".dml"; - programArgs = new String[]{ "-stats","-args", String.valueOf(N), output("R") }; - OptimizerUtils.ALLOW_ALGEBRAIC_SIMPLIFICATION = rewrites; - rtplatform = RUNTIME_PLATFORM.SINGLE_NODE; - - runTest(true, false, null, -1); - HashMap<CellIndex, Double> dmlfile = readDMLMatrixFromHDFS("R"); - Assert.assertTrue( dmlfile.get(new CellIndex(1,1))==2*(N+2) ); - } - finally { - OptimizerUtils.ALLOW_ALGEBRAIC_SIMPLIFICATION = oldFlag; - rtplatform = oldPlatform; - } - } -} http://git-wip-us.apache.org/repos/asf/systemml/blob/b85aea34/src/test/java/org/apache/sysml/test/integration/functions/misc/SizePropagationTest.java ---------------------------------------------------------------------- diff --git a/src/test/java/org/apache/sysml/test/integration/functions/misc/SizePropagationTest.java b/src/test/java/org/apache/sysml/test/integration/functions/misc/SizePropagationTest.java new file mode 100644 index 0000000..4a8ed98 --- /dev/null +++ b/src/test/java/org/apache/sysml/test/integration/functions/misc/SizePropagationTest.java @@ -0,0 +1,107 @@ +/* + * 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.junit.Assert; + +import java.util.HashMap; + +import org.apache.sysml.api.DMLScript.RUNTIME_PLATFORM; +import org.apache.sysml.hops.OptimizerUtils; +import org.apache.sysml.runtime.matrix.data.MatrixValue.CellIndex; +import org.apache.sysml.test.integration.AutomatedTestBase; +import org.apache.sysml.test.integration.TestConfiguration; +import org.apache.sysml.test.utils.TestUtils; + +public class SizePropagationTest extends AutomatedTestBase +{ + private static final String TEST_NAME1 = "SizePropagationRBind"; + private static final String TEST_NAME2 = "SizePropagationLoopIx1"; + private static final String TEST_NAME3 = "SizePropagationLoopIx2"; + + private static final String TEST_DIR = "functions/misc/"; + private static final String TEST_CLASS_DIR = TEST_DIR + SizePropagationTest.class.getSimpleName() + "/"; + + private static final int N = 100; + + @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" }) ); + addTestConfiguration( TEST_NAME3, new TestConfiguration(TEST_CLASS_DIR, TEST_NAME3, new String[] { "R" }) ); + } + + @Test + public void testSizePropagationRBindNoRewrites() { + testSizePropagation( TEST_NAME1, false, 2*(N+2) ); + } + + @Test + public void testSizePropagationRBindRewrites() { + testSizePropagation( TEST_NAME1, true, 2*(N+2) ); + } + + @Test + public void testSizePropagationLoopIx1NoRewrites() { + testSizePropagation( TEST_NAME2, false, N ); + } + + @Test + public void testSizePropagationLoopIx1Rewrites() { + testSizePropagation( TEST_NAME2, true, N ); + } + + @Test + public void testSizePropagationLoopIx2NoRewrites() { + testSizePropagation( TEST_NAME3, false, N-2 ); + } + + @Test + public void testSizePropagationLoopIx2Rewrites() { + testSizePropagation( TEST_NAME3, true, N-2 ); + } + + private void testSizePropagation( String testname, boolean rewrites, int expect ) { + boolean oldFlag = OptimizerUtils.ALLOW_ALGEBRAIC_SIMPLIFICATION; + RUNTIME_PLATFORM oldPlatform = rtplatform; + + try { + TestConfiguration config = getTestConfiguration(testname); + loadTestConfiguration(config); + + String HOME = SCRIPT_DIR + TEST_DIR; + fullDMLScriptName = HOME + testname + ".dml"; + programArgs = new String[]{ "-explain", "hops", "-stats","-args", String.valueOf(N), output("R") }; + OptimizerUtils.ALLOW_ALGEBRAIC_SIMPLIFICATION = rewrites; + rtplatform = RUNTIME_PLATFORM.SINGLE_NODE; + + runTest(true, false, null, -1); + HashMap<CellIndex, Double> dmlfile = readDMLMatrixFromHDFS("R"); + Assert.assertEquals(new Double(expect), dmlfile.get(new CellIndex(1,1))); + } + finally { + OptimizerUtils.ALLOW_ALGEBRAIC_SIMPLIFICATION = oldFlag; + rtplatform = oldPlatform; + } + } +} http://git-wip-us.apache.org/repos/asf/systemml/blob/b85aea34/src/test/java/org/apache/sysml/test/integration/functions/recompile/FunctionRecompileTest.java ---------------------------------------------------------------------- diff --git a/src/test/java/org/apache/sysml/test/integration/functions/recompile/FunctionRecompileTest.java b/src/test/java/org/apache/sysml/test/integration/functions/recompile/FunctionRecompileTest.java index 98a93ad..55a2d5b 100644 --- a/src/test/java/org/apache/sysml/test/integration/functions/recompile/FunctionRecompileTest.java +++ b/src/test/java/org/apache/sysml/test/integration/functions/recompile/FunctionRecompileTest.java @@ -33,49 +33,41 @@ import org.apache.sysml.utils.Statistics; public class FunctionRecompileTest extends AutomatedTestBase { - private final static String TEST_NAME1 = "funct_recompile"; private final static String TEST_DIR = "functions/recompile/"; private final static String TEST_CLASS_DIR = TEST_DIR + FunctionRecompileTest.class.getSimpleName() + "/"; private final static double eps = 1e-10; private final static int rows = 20; - private final static int cols = 10; + private final static int cols = 10; private final static double sparsity = 1.0; - @Override - public void setUp() - { + public void setUp() { TestUtils.clearAssertionInformation(); addTestConfiguration(TEST_NAME1, new TestConfiguration(TEST_CLASS_DIR, TEST_NAME1, new String[] { "Rout" }) ); } @Test - public void testFunctionWithoutRecompileWithoutIPA() - { + public void testFunctionWithoutRecompileWithoutIPA() { runFunctionTest(false, false); } @Test - public void testFunctionWithoutRecompileWithIPA() - { + public void testFunctionWithoutRecompileWithIPA() { runFunctionTest(false, true); } @Test - public void testFunctionWithRecompileWithoutIPA() - { + public void testFunctionWithRecompileWithoutIPA() { runFunctionTest(true, false); } @Test - public void testFunctionWithRecompileWithIPA() - { + public void testFunctionWithRecompileWithIPA() { runFunctionTest(true, true); } - private void runFunctionTest( boolean recompile, boolean IPA ) { @@ -89,17 +81,16 @@ public class FunctionRecompileTest extends AutomatedTestBase config.addVariable("cols", cols); loadTestConfiguration(config); - /* This is for running the junit test the new way, i.e., construct the arguments directly */ String HOME = SCRIPT_DIR + TEST_DIR; fullDMLScriptName = HOME + TEST_NAME1 + ".dml"; - programArgs = new String[]{"-args", input("V"), + programArgs = new String[]{"-explain", "-args", input("V"), Integer.toString(rows), Integer.toString(cols), output("R") }; fullRScriptName = HOME + TEST_NAME1 + ".R"; rCmd = "Rscript" + " " + fullRScriptName + " " + inputDir() + " " + expectedDir(); long seed = System.nanoTime(); - double[][] V = getRandomMatrix(rows, cols, 0, 1, sparsity, seed); + double[][] V = getRandomMatrix(rows, cols, 0, 1, sparsity, seed); writeInputMatrix("V", V, true); CompilerConfig.FLAG_DYN_RECOMPILE = recompile; @@ -112,29 +103,27 @@ public class FunctionRecompileTest extends AutomatedTestBase //note: change from previous version due to fix in op selection (unknown size XtX and mapmult) //CHECK compiled MR jobs int expectNumCompiled = -1; - if( IPA ) expectNumCompiled = 1; //reblock (with recompile right indexing); before: 3 reblock, GMR,GMR + if( IPA ) expectNumCompiled = 4; //reblock TODO investigate 1-4 recompile side effect else expectNumCompiled = 5;//reblock, GMR,GMR,GMR,GMR (last two should piggybacked) Assert.assertEquals("Unexpected number of compiled MR jobs.", - expectNumCompiled, Statistics.getNoOfCompiledMRJobs()); + expectNumCompiled, Statistics.getNoOfCompiledMRJobs()); //CHECK executed MR jobs int expectNumExecuted = -1; if( recompile ) expectNumExecuted = 0; - else if( IPA ) expectNumExecuted = 1; //reblock (with recompile right indexing); before: 21 reblock, 10*(GMR,GMR) + else if( IPA ) expectNumExecuted = 31; //reblock TODO investigate 1-31 recompile side effect else expectNumExecuted = 41; //reblock, 10*(GMR,GMR,GMR, GMR) (last two should piggybacked) Assert.assertEquals("Unexpected number of executed MR jobs.", - expectNumExecuted, Statistics.getNoOfExecutedMRJobs()); + expectNumExecuted, Statistics.getNoOfExecutedMRJobs()); //compare matrices HashMap<CellIndex, Double> dmlfile = readDMLMatrixFromHDFS("R"); HashMap<CellIndex, Double> rfile = readRMatrixFromFS("Rout"); - TestUtils.compareMatrices(dmlfile, rfile, eps, "DML", "R"); + TestUtils.compareMatrices(dmlfile, rfile, eps, "DML", "R"); } - finally - { + finally { CompilerConfig.FLAG_DYN_RECOMPILE = oldFlagRecompile; OptimizerUtils.ALLOW_INTER_PROCEDURAL_ANALYSIS = oldFlagIPA; } } - -} \ No newline at end of file +} http://git-wip-us.apache.org/repos/asf/systemml/blob/b85aea34/src/test/scripts/functions/misc/SizePropagationLoopIx1.dml ---------------------------------------------------------------------- diff --git a/src/test/scripts/functions/misc/SizePropagationLoopIx1.dml b/src/test/scripts/functions/misc/SizePropagationLoopIx1.dml new file mode 100644 index 0000000..1daf730 --- /dev/null +++ b/src/test/scripts/functions/misc/SizePropagationLoopIx1.dml @@ -0,0 +1,31 @@ +#------------------------------------------------------------- +# +# 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. +# +#------------------------------------------------------------- + +X = rand(rows=$1, cols=1); +Y = X +# no loop iterations: n=$1 +for( i in seq(1,0,1) ) { + n1 = nrow(Y) + 0.0; + Y = Y[2:n1,] - Y[1:n1-1,]; +} +n = nrow(Y); +R = as.matrix(n); +write(R, $2); http://git-wip-us.apache.org/repos/asf/systemml/blob/b85aea34/src/test/scripts/functions/misc/SizePropagationLoopIx2.dml ---------------------------------------------------------------------- diff --git a/src/test/scripts/functions/misc/SizePropagationLoopIx2.dml b/src/test/scripts/functions/misc/SizePropagationLoopIx2.dml new file mode 100644 index 0000000..d791699 --- /dev/null +++ b/src/test/scripts/functions/misc/SizePropagationLoopIx2.dml @@ -0,0 +1,31 @@ +#------------------------------------------------------------- +# +# 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. +# +#------------------------------------------------------------- + +X = rand(rows=$1, cols=1); +Y = X +# two loop iterations: n=$1-2 +for( i in seq(1,2,1) ) { + n1 = nrow(Y) + 0.0; + Y = Y[2:n1,] - Y[1:n1-1,]; +} +n = nrow(Y); +R = as.matrix(n); +write(R, $2); http://git-wip-us.apache.org/repos/asf/systemml/blob/b85aea34/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 ab232d2..8d3f7f3 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 @@ -78,7 +78,7 @@ import org.junit.runners.Suite; ScalarMatrixUnaryBinaryTermTest.class, ScalarToMatrixInLoopTest.class, SetWorkingDirTest.class, - SizePropagationRBindTest.class, + SizePropagationTest.class, ToStringTest.class, ValueTypeAutoCastingTest.class, ValueTypeCastingTest.class,
