You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@systemml.apache.org by mb...@apache.org on 2017/10/24 06:43:31 UTC
[2/2] systemml git commit: [SYSTEMML-1972] Fix rewrite remove right
indexing (w/ invalid ix range)
[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 <mb...@gmail.com>
Authored: Mon Oct 23 23:43:46 2017 -0700
Committer: Matthias Boehm <mb...@gmail.com>
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;