[SYSTEMML-1972] Fix rewrite remove right indexing (w/ invalid ix range) This patch hardens the existing rewrite of removing unnecessary right indexing operations whose input and output are of equal size, which is only valid with valid indexing ranges. Although we check this during validation, there are scenarios with unknown sizes or index expressions that cause invalid results despite invalid index ranges. We now simply check for valid row-lower and column-lower indexing ranges which both needs to be 1 for the rewrite to be valid.
Project: http://git-wip-us.apache.org/repos/asf/systemml/repo Commit: http://git-wip-us.apache.org/repos/asf/systemml/commit/a472ae92 Tree: http://git-wip-us.apache.org/repos/asf/systemml/tree/a472ae92 Diff: http://git-wip-us.apache.org/repos/asf/systemml/diff/a472ae92 Branch: refs/heads/master Commit: a472ae922827b437e00ca8331ff3db5f6c19f443 Parents: 2c37d9f Author: Matthias Boehm <[email protected]> Authored: Mon Oct 23 23:43:46 2017 -0700 Committer: Matthias Boehm <[email protected]> Committed: Mon Oct 23 23:44:04 2017 -0700 ---------------------------------------------------------------------- .../java/org/apache/sysml/hops/IndexingOp.java | 5 +---- .../sysml/hops/rewrite/HopRewriteUtils.java | 13 +++++++++++ .../RewriteAlgebraicSimplificationDynamic.java | 23 ++++++-------------- 3 files changed, 21 insertions(+), 20 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/systemml/blob/a472ae92/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 23d0630..5989c66 100644 --- a/src/main/java/org/apache/sysml/hops/IndexingOp.java +++ b/src/main/java/org/apache/sysml/hops/IndexingOp.java @@ -118,10 +118,7 @@ public class IndexingOp extends Hop Hop input = getInput().get(0); //rewrite remove unnecessary right indexing - if( dimsKnown() && input.dimsKnown() - && getDim1() == input.getDim1() && getDim2() == input.getDim2() - && !(getDim1()==1 && getDim2()==1)) - { + if( HopRewriteUtils.isUnnecessaryRightIndexing(this) ) { setLops( input.constructLops() ); } //actual lop construction, incl operator selection http://git-wip-us.apache.org/repos/asf/systemml/blob/a472ae92/src/main/java/org/apache/sysml/hops/rewrite/HopRewriteUtils.java ---------------------------------------------------------------------- diff --git a/src/main/java/org/apache/sysml/hops/rewrite/HopRewriteUtils.java b/src/main/java/org/apache/sysml/hops/rewrite/HopRewriteUtils.java index 68068eb..ad2392a 100644 --- a/src/main/java/org/apache/sysml/hops/rewrite/HopRewriteUtils.java +++ b/src/main/java/org/apache/sysml/hops/rewrite/HopRewriteUtils.java @@ -1000,6 +1000,19 @@ public class HopRewriteUtils && hop.getInput().get(4) instanceof LiteralOp; } + public static boolean isUnnecessaryRightIndexing(Hop hop) { + if( !(hop instanceof IndexingOp) ) + return false; + //note: in addition to equal sizes, we also check a valid + //starting row and column ranges of 1 in order to guard against + //invalid modifications in the presence of invalid index ranges + //(e.g., X[,2] on a column vector needs to throw an error) + return isEqualSize(hop, hop.getInput().get(0)) + && !(hop.getDim1()==1 && hop.getDim2()==1) + && isLiteralOfValue(hop.getInput().get(1), 1) //rl + && isLiteralOfValue(hop.getInput().get(3), 1); //cl + } + public static boolean isScalarMatrixBinaryMult( Hop hop ) { return hop instanceof BinaryOp && ((BinaryOp)hop).getOp()==OpOp2.MULT && ((hop.getInput().get(0).getDataType()==DataType.SCALAR && hop.getInput().get(1).getDataType()==DataType.MATRIX) http://git-wip-us.apache.org/repos/asf/systemml/blob/a472ae92/src/main/java/org/apache/sysml/hops/rewrite/RewriteAlgebraicSimplificationDynamic.java ---------------------------------------------------------------------- diff --git a/src/main/java/org/apache/sysml/hops/rewrite/RewriteAlgebraicSimplificationDynamic.java b/src/main/java/org/apache/sysml/hops/rewrite/RewriteAlgebraicSimplificationDynamic.java index 5437535..eba06fc 100644 --- a/src/main/java/org/apache/sysml/hops/rewrite/RewriteAlgebraicSimplificationDynamic.java +++ b/src/main/java/org/apache/sysml/hops/rewrite/RewriteAlgebraicSimplificationDynamic.java @@ -230,23 +230,14 @@ public class RewriteAlgebraicSimplificationDynamic extends HopRewriteRule private static Hop removeUnnecessaryRightIndexing(Hop parent, Hop hi, int pos) { - if( hi instanceof IndexingOp ) //indexing op - { + if( HopRewriteUtils.isUnnecessaryRightIndexing(hi) ) { + //remove unnecessary right indexing Hop input = hi.getInput().get(0); - if( HopRewriteUtils.isEqualSize(hi, input) //equal dims - && !(hi.getDim1()==1 && hi.getDim2()==1) ) //not 1-1 matrix/frame - { - //equal dims of right indexing input and output -> no need for indexing - //(not applied for 1-1 matrices because low potential and issues w/ error - //handling if out of range indexing) - - //remove unnecessary right indexing - HopRewriteUtils.replaceChildReference(parent, hi, input, pos); - HopRewriteUtils.cleanupUnreferenced(hi); - hi = input; - - LOG.debug("Applied removeUnnecessaryRightIndexing"); - } + HopRewriteUtils.replaceChildReference(parent, hi, input, pos); + HopRewriteUtils.cleanupUnreferenced(hi); + hi = input; + + LOG.debug("Applied removeUnnecessaryRightIndexing"); } return hi;
