From 3e1d1055d13b55a4bce8288a901299d681185448 Mon Sep 17 00:00:00 2001 From: David Leopoldseder Date: Thu, 18 Jan 2024 16:09:11 +0100 Subject: [PATCH 1/4] use safe abs in loop opts (cherry picked from commit 28b5bd64aaa9e7dad68acc7ddccb466e0dbf43fb) --- .../loop/phases/LoopPredicationPhase.java | 2 ++ .../jdk/graal/compiler/nodes/loop/LoopEx.java | 15 +++++++-- .../phases/common/util/LoopUtility.java | 33 +++++++++++++++++++ 3 files changed, 48 insertions(+), 2 deletions(-) diff --git a/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/loop/phases/LoopPredicationPhase.java b/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/loop/phases/LoopPredicationPhase.java index 7c44206b9671..f6769e558689 100644 --- a/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/loop/phases/LoopPredicationPhase.java +++ b/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/loop/phases/LoopPredicationPhase.java @@ -114,6 +114,8 @@ protected void run(StructuredGraph graph, MidTierContext context) { final boolean inverted = loop.counted().isInverted(); if ((((IntegerStamp) counter.valueNode().stamp(NodeView.DEFAULT)).getBits() == 32) && !counted.isUnsignedCheck() && + // math.abs can overflow here but only to min again which is + // never == 1 ((condition != NE && condition != EQ) || (counter.isConstantStride() && Math.abs(counter.constantStride()) == 1)) && (loop.loopBegin().isMainLoop() || loop.loopBegin().isSimpleLoop())) { NodeIterable guards = loop.whole().nodes().filter(GuardNode.class); diff --git a/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/nodes/loop/LoopEx.java b/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/nodes/loop/LoopEx.java index 2a5ad5e71fc2..48e32d0d38e2 100644 --- a/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/nodes/loop/LoopEx.java +++ b/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/nodes/loop/LoopEx.java @@ -79,6 +79,7 @@ import jdk.graal.compiler.nodes.extended.ValueAnchorNode; import jdk.graal.compiler.nodes.util.GraphUtil; import jdk.graal.compiler.phases.common.CanonicalizerPhase; +import jdk.graal.compiler.phases.common.util.LoopUtility; public class LoopEx { protected final Loop loop; @@ -334,7 +335,7 @@ public boolean detectCounted() { // signed: i < MAX_INT } else if (limitStamp.asConstant() != null && limitStamp.asConstant().asLong() == counterStamp.unsignedUpperBound()) { unsigned = true; - } else if (!iv.isConstantStride() || Math.abs(iv.constantStride()) != 1 || initStamp.upperBound() > limitStamp.lowerBound()) { + } else if (!iv.isConstantStride() || !absStrideIsOne(iv) || initStamp.upperBound() > limitStamp.lowerBound()) { return false; } } else if (iv.direction() == InductionVariable.Direction.Down) { @@ -342,7 +343,7 @@ public boolean detectCounted() { // signed: MIN_INT > i } else if (limitStamp.asConstant() != null && limitStamp.asConstant().asLong() == counterStamp.unsignedLowerBound()) { unsigned = true; - } else if (!iv.isConstantStride() || Math.abs(iv.constantStride()) != 1 || initStamp.lowerBound() < limitStamp.upperBound()) { + } else if (!iv.isConstantStride() || !absStrideIsOne(iv) || initStamp.lowerBound() < limitStamp.upperBound()) { return false; } } else { @@ -389,6 +390,16 @@ public boolean detectCounted() { return false; } + public static boolean absStrideIsOne(InductionVariable limitCheckedIV) { + final long absStride; + try { + absStride = LoopUtility.abs(limitCheckedIV.constantStride(), IntegerStamp.getBits(limitCheckedIV.strideNode().stamp(NodeView.DEFAULT))); + } catch (ArithmeticException e) { + return false; + } + return absStride == 1; + } + public boolean isCfgLoopExit(AbstractBeginNode begin) { HIRBlock block = data.getCFG().blockFor(begin); return loop.getDepth() > block.getLoopDepth() || loop.isNaturalExit(block); diff --git a/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/phases/common/util/LoopUtility.java b/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/phases/common/util/LoopUtility.java index 8fe102300bab..c3cfa1b04fbd 100644 --- a/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/phases/common/util/LoopUtility.java +++ b/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/phases/common/util/LoopUtility.java @@ -29,6 +29,7 @@ import jdk.graal.compiler.core.common.cfg.Loop; import jdk.graal.compiler.core.common.type.IntegerStamp; import jdk.graal.compiler.core.common.type.Stamp; +import jdk.graal.compiler.debug.GraalError; import jdk.graal.compiler.graph.Graph.NodeEvent; import jdk.graal.compiler.graph.Graph.NodeEventScope; import jdk.graal.compiler.graph.Node; @@ -54,6 +55,38 @@ public class LoopUtility { + public static boolean canTakeAbs(long l, int bits) { + try { + abs(l, bits); + return true; + } catch (ArithmeticException e) { + return false; + } + } + + /** + * Compute {@link Math#abs(long)} for the given arguments and the given bit size. Throw a + * {@link ArithmeticException} if the abs operation would overflow. + */ + public static long abs(long l, int bits) throws ArithmeticException { + if (bits == 32) { + if (l == Integer.MIN_VALUE) { + throw new ArithmeticException("Abs on Integer.MIN_VALUE would cause an overflow because abs(Integer.MIN_VALUE) = Integer.MAX_VALUE + 1 which does not fit in int (32bits)"); + } else { + final int i = (int) l; + return Math.abs(i); + } + } else if (bits == 64) { + if (l == Long.MIN_VALUE) { + throw new ArithmeticException("Abs on Long.MIN_VALUE would cause an overflow because abs(Long.MIN_VALUE) = Long.MAX_VALUE + 1 which does not fit in int (32bits)"); + } else { + return Math.abs(l); + } + } else { + throw GraalError.shouldNotReachHere("Must be one of java's core datatypes int/long but is " + bits); + } + } + /** * Determine if the def can use node {@code use} without the need for value proxies. This means * there is no loop exit between the schedule point of def and use that would require a From 4e6249a2c0aa058c55133bf8f11c232c89b658b1 Mon Sep 17 00:00:00 2001 From: David Leopoldseder Date: Tue, 23 Jan 2024 13:47:56 +0100 Subject: [PATCH 2/4] fix comment (cherry picked from commit 83c699edbfc6af7b6effeb3ad536fbc6531edf3a) --- .../jdk/graal/compiler/phases/common/util/LoopUtility.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/phases/common/util/LoopUtility.java b/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/phases/common/util/LoopUtility.java index c3cfa1b04fbd..f025087619d8 100644 --- a/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/phases/common/util/LoopUtility.java +++ b/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/phases/common/util/LoopUtility.java @@ -71,14 +71,14 @@ public static boolean canTakeAbs(long l, int bits) { public static long abs(long l, int bits) throws ArithmeticException { if (bits == 32) { if (l == Integer.MIN_VALUE) { - throw new ArithmeticException("Abs on Integer.MIN_VALUE would cause an overflow because abs(Integer.MIN_VALUE) = Integer.MAX_VALUE + 1 which does not fit in int (32bits)"); + throw new ArithmeticException("Abs on Integer.MIN_VALUE would cause an overflow because abs(Integer.MIN_VALUE) = Integer.MAX_VALUE + 1 which does not fit in int (32 bits)"); } else { final int i = (int) l; return Math.abs(i); } } else if (bits == 64) { if (l == Long.MIN_VALUE) { - throw new ArithmeticException("Abs on Long.MIN_VALUE would cause an overflow because abs(Long.MIN_VALUE) = Long.MAX_VALUE + 1 which does not fit in int (32bits)"); + throw new ArithmeticException("Abs on Long.MIN_VALUE would cause an overflow because abs(Long.MIN_VALUE) = Long.MAX_VALUE + 1 which does not fit in long (64 bits)"); } else { return Math.abs(l); } From 212002dcab2af01e5d58f07fbf3f11f357c5230d Mon Sep 17 00:00:00 2001 From: David Leopoldseder Date: Tue, 23 Jan 2024 13:55:27 +0100 Subject: [PATCH 3/4] loop ex: allow overflow in math.abs for stride !=1 checks, it does not matter, in case of overflow the value is still different (cherry picked from commit ac0fc684c9b78b337873d5789d8886cd6a93cea9) --- .../src/jdk/graal/compiler/nodes/loop/LoopEx.java | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/nodes/loop/LoopEx.java b/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/nodes/loop/LoopEx.java index 48e32d0d38e2..7b349523b243 100644 --- a/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/nodes/loop/LoopEx.java +++ b/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/nodes/loop/LoopEx.java @@ -79,7 +79,6 @@ import jdk.graal.compiler.nodes.extended.ValueAnchorNode; import jdk.graal.compiler.nodes.util.GraphUtil; import jdk.graal.compiler.phases.common.CanonicalizerPhase; -import jdk.graal.compiler.phases.common.util.LoopUtility; public class LoopEx { protected final Loop loop; @@ -391,13 +390,12 @@ public boolean detectCounted() { } public static boolean absStrideIsOne(InductionVariable limitCheckedIV) { - final long absStride; - try { - absStride = LoopUtility.abs(limitCheckedIV.constantStride(), IntegerStamp.getBits(limitCheckedIV.strideNode().stamp(NodeView.DEFAULT))); - } catch (ArithmeticException e) { - return false; - } - return absStride == 1; + /* + * While Math.abs can overflow for MIN_VALUE it is fine here. In case of overflow we still + * get a value != 1 (namely MIN_VALUE again). Overflow handling for the limit checked IV is + * done in CountedLoopInfo and is an orthogonal issue. + */ + return Math.abs(limitCheckedIV.constantStride()) == 1; } public boolean isCfgLoopExit(AbstractBeginNode begin) { From 917983db31ba28ccaafa3197574e75127692a4b6 Mon Sep 17 00:00:00 2001 From: David Leopoldseder Date: Fri, 26 Jan 2024 14:30:23 +0100 Subject: [PATCH 4/4] reuse loopex.absStrideIsOne (cherry picked from commit 429b3f9fb983e98bd87b8d440c43195e4d75f0d1) --- .../graal/compiler/loop/phases/LoopPredicationPhase.java | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/loop/phases/LoopPredicationPhase.java b/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/loop/phases/LoopPredicationPhase.java index f6769e558689..4fbf9d85c95e 100644 --- a/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/loop/phases/LoopPredicationPhase.java +++ b/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/loop/phases/LoopPredicationPhase.java @@ -112,11 +112,8 @@ protected void run(StructuredGraph graph, MidTierContext context) { final InductionVariable counter = counted.getLimitCheckedIV(); final Condition condition = ((CompareNode) counted.getLimitTest().condition()).condition().asCondition(); final boolean inverted = loop.counted().isInverted(); - if ((((IntegerStamp) counter.valueNode().stamp(NodeView.DEFAULT)).getBits() == 32) && - !counted.isUnsignedCheck() && - // math.abs can overflow here but only to min again which is - // never == 1 - ((condition != NE && condition != EQ) || (counter.isConstantStride() && Math.abs(counter.constantStride()) == 1)) && + if ((((IntegerStamp) counter.valueNode().stamp(NodeView.DEFAULT)).getBits() == 32) && !counted.isUnsignedCheck() && + ((condition != NE && condition != EQ) || (counter.isConstantStride() && LoopEx.absStrideIsOne(counter))) && (loop.loopBegin().isMainLoop() || loop.loopBegin().isSimpleLoop())) { NodeIterable guards = loop.whole().nodes().filter(GuardNode.class); if (LoopPredicationMainPath.getValue(graph.getOptions())) {