[GR-51012] Avoid losing stamp precision during array length lowering.

PullRequest: graal/16425
chrischiedo · Dec 23, 2023 · 8c22496 · 8c22496
2 parents 25a40ea + cdc813a
commit 8c22496
Show file tree

Hide file tree

Showing 8 changed files with 126 additions and 14 deletions.
diff --git a/...rc/jdk.graal.compiler.test/src/jdk/graal/compiler/nodes/test/ConstantArrayLengthTest.java b/...rc/jdk.graal.compiler.test/src/jdk/graal/compiler/nodes/test/ConstantArrayLengthTest.java
@@ -0,0 +1,56 @@
+/*
+ * Copyright (c) 2023, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.  Oracle designates this
+ * particular file as subject to the "Classpath" exception as provided
+ * by Oracle in the LICENSE file that accompanied this code.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+package jdk.graal.compiler.nodes.test;
+
+import org.junit.Test;
+
+import jdk.graal.compiler.core.test.GraalCompilerTest;
+import jdk.graal.compiler.options.OptionValues;
+
+public class ConstantArrayLengthTest extends GraalCompilerTest {
+
+    static int method0(int[] arg, int a, int b) {
+        int len = 0;
+        int[] arr = null;
+        if (a == noBCInline(a)) {
+            arr = new int[Math.addExact(a, b)];
+        } else {
+            arr = arg;
+        }
+        len = arr.length;
+        return len / a;
+    }
+
+    @BytecodeParserNeverInline
+    public static int noBCInline(int i) {
+        return i;
+    }
+
+    @Test
+    public void test0() {
+        OptionValues opt = getInitialOptions();
+        test(opt, "method0", new int[]{}, 100, 10);
+    }
+}
diff --git a/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/core/CompilationWrapper.java b/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/core/CompilationWrapper.java
@@ -335,8 +335,16 @@ protected T handleFailure(DebugContext initialDebug, Throwable cause) {
                 TTY.printf("Error writing to %s: %s%n", retryLogFile, ioe);
             }
 
+            String diagnoseLevel = DebugOptions.DiagnoseDumpLevel.getValue(initialOptions);
+
+            // pre GR-51012 this was just a number - we still want to support the old numeric values
+            boolean isOldLevel = diagnoseLevel.matches("-?\\d+");
+            if (isOldLevel) {
+                diagnoseLevel = ":" + diagnoseLevel;
+            }
+
             OptionValues retryOptions = new OptionValues(initialOptions,
-                            Dump, ":" + DebugOptions.DiagnoseDumpLevel.getValue(initialOptions),
+                            Dump, diagnoseLevel,
                             MethodFilter, null,
                             Count, "",
                             Time, "",

diff --git a/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/debug/DebugOptions.java b/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/debug/DebugOptions.java
@@ -147,7 +147,7 @@ public enum OptimizationLogTarget {
     public static final OptionKey<Boolean> DumpOnError = new OptionKey<>(false);
     @Option(help = "Specify the dump level if CompilationFailureAction#Diagnose is used." +
                     "See CompilationFailureAction for details. file:doc-files/CompilationFailureActionHelp.txt", type = OptionType.Debug)
-    public static final OptionKey<Integer> DiagnoseDumpLevel = new OptionKey<>(DebugContext.VERBOSE_LEVEL);
+    public static final OptionKey<String> DiagnoseDumpLevel = new OptionKey<>(":" + DebugContext.VERBOSE_LEVEL);
     @Option(help = "Disable intercepting exceptions in debug scopes.", type = OptionType.Debug)
     public static final OptionKey<Boolean> DisableIntercept = new OptionKey<>(false);
     @Option(help = "Intercept also bailout exceptions", type = OptionType.Debug)

diff --git a/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/nodes/java/ArrayLengthNode.java b/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/nodes/java/ArrayLengthNode.java
@@ -38,12 +38,13 @@
 import jdk.graal.compiler.nodes.ConstantNode;
 import jdk.graal.compiler.nodes.DeoptimizeNode;
 import jdk.graal.compiler.nodes.FixedWithNextNode;
+import jdk.graal.compiler.nodes.GraphState.StageFlag;
 import jdk.graal.compiler.nodes.NamedLocationIdentity;
 import jdk.graal.compiler.nodes.NodeView;
 import jdk.graal.compiler.nodes.PiNode;
 import jdk.graal.compiler.nodes.StructuredGraph;
 import jdk.graal.compiler.nodes.ValueNode;
-import jdk.graal.compiler.nodes.GraphState.StageFlag;
+import jdk.graal.compiler.nodes.extended.ValueAnchorNode;
 import jdk.graal.compiler.nodes.graphbuilderconf.GraphBuilderContext;
 import jdk.graal.compiler.nodes.memory.MemoryAccess;
 import jdk.graal.compiler.nodes.spi.ArrayLengthProvider;
@@ -171,7 +172,7 @@ public void simplify(SimplifierTool tool) {
             StructuredGraph graph = graph();
             ValueNode replacement = length;
             if (!length.isConstant() && length.stamp(NodeView.DEFAULT).canBeImprovedWith(StampFactory.positiveInt())) {
-                BeginNode guard = graph.add(new BeginNode());
+                ValueAnchorNode guard = graph.add(new ValueAnchorNode());
                 graph.addAfterFixed(this, guard);
                 replacement = graph.addWithoutUnique(new PiNode(length, StampFactory.positiveInt(), guard));
             }

diff --git a/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/nodes/memory/FloatingReadNode.java b/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/nodes/memory/FloatingReadNode.java
@@ -28,6 +28,8 @@
 import static jdk.graal.compiler.nodeinfo.NodeCycles.CYCLES_2;
 import static jdk.graal.compiler.nodeinfo.NodeSize.SIZE_1;
 
+import org.graalvm.word.LocationIdentity;
+
 import jdk.graal.compiler.core.common.LIRKind;
 import jdk.graal.compiler.core.common.memory.BarrierType;
 import jdk.graal.compiler.core.common.memory.MemoryExtendKind;
@@ -38,14 +40,13 @@
 import jdk.graal.compiler.graph.Node;
 import jdk.graal.compiler.graph.NodeClass;
 import jdk.graal.compiler.nodeinfo.NodeInfo;
+import jdk.graal.compiler.nodes.NodeView;
+import jdk.graal.compiler.nodes.ValuePhiNode;
 import jdk.graal.compiler.nodes.extended.GuardingNode;
+import jdk.graal.compiler.nodes.memory.address.AddressNode;
 import jdk.graal.compiler.nodes.spi.Canonicalizable;
 import jdk.graal.compiler.nodes.spi.CanonicalizerTool;
 import jdk.graal.compiler.nodes.spi.NodeLIRBuilderTool;
-import jdk.graal.compiler.nodes.NodeView;
-import jdk.graal.compiler.nodes.ValuePhiNode;
-import jdk.graal.compiler.nodes.memory.address.AddressNode;
-import org.graalvm.word.LocationIdentity;
 
 /**
  * A floating read of a value from memory specified in terms of an object base and an object
@@ -134,4 +135,5 @@ public boolean verify() {
     public Stamp getAccessStamp(NodeView view) {
         return stamp(view);
     }
+
 }
diff --git a/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/nodes/memory/ReadNode.java b/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/nodes/memory/ReadNode.java
@@ -51,17 +51,22 @@
 import jdk.graal.compiler.nodes.FixedWithNextNode;
 import jdk.graal.compiler.nodes.FrameState;
 import jdk.graal.compiler.nodes.NodeView;
+import jdk.graal.compiler.nodes.PiNode;
+import jdk.graal.compiler.nodes.StructuredGraph;
 import jdk.graal.compiler.nodes.ValueNode;
 import jdk.graal.compiler.nodes.calc.NarrowNode;
 import jdk.graal.compiler.nodes.calc.ZeroExtendNode;
 import jdk.graal.compiler.nodes.extended.GuardingNode;
+import jdk.graal.compiler.nodes.extended.ValueAnchorNode;
 import jdk.graal.compiler.nodes.memory.address.AddressNode;
 import jdk.graal.compiler.nodes.memory.address.OffsetAddressNode;
 import jdk.graal.compiler.nodes.spi.ArrayLengthProvider;
 import jdk.graal.compiler.nodes.spi.Canonicalizable;
 import jdk.graal.compiler.nodes.spi.CanonicalizerTool;
 import jdk.graal.compiler.nodes.spi.CoreProviders;
 import jdk.graal.compiler.nodes.spi.NodeLIRBuilderTool;
+import jdk.graal.compiler.nodes.spi.Simplifiable;
+import jdk.graal.compiler.nodes.spi.SimplifierTool;
 import jdk.graal.compiler.nodes.spi.Virtualizable;
 import jdk.graal.compiler.nodes.spi.VirtualizerTool;
 import jdk.graal.compiler.nodes.util.ConstantFoldUtil;
@@ -76,7 +81,8 @@
  * Reads an {@linkplain FixedAccessNode accessed} value.
  */
 @NodeInfo(nameTemplate = "Read#{p#location/s}", cycles = CYCLES_2, size = SIZE_1)
-public class ReadNode extends FloatableAccessNode implements LIRLowerableAccess, Canonicalizable, Virtualizable, GuardingNode, OrderedMemoryAccess, SingleMemoryKill, ExtendableMemoryAccess {
+public class ReadNode extends FloatableAccessNode
+                implements LIRLowerableAccess, Canonicalizable, Virtualizable, GuardingNode, OrderedMemoryAccess, SingleMemoryKill, ExtendableMemoryAccess, Simplifiable {
 
     public static final NodeClass<ReadNode> TYPE = NodeClass.create(ReadNode.class);
 
@@ -126,13 +132,17 @@ public void generate(NodeLIRBuilderTool gen) {
         }
     }
 
+    private boolean canCanonicalizeRead() {
+        return !getUsedAsNullCheck() && !extendsAccess();
+    }
+
     @Override
     public Node canonical(CanonicalizerTool tool) {
         if (tool.allUsagesAvailable() && hasNoUsages()) {
             // Read without usages or guard can be safely removed.
             return null;
         }
-        if (!getUsedAsNullCheck() && !extendsAccess()) {
+        if (canCanonicalizeRead()) {
             return canonicalizeRead(this, getAddress(), getLocationIdentity(), tool);
         } else {
             // if this read is a null check, then replacing it with the value is incorrect for
@@ -141,6 +151,36 @@ public Node canonical(CanonicalizerTool tool) {
         }
     }
 
+    @Override
+    public void simplify(SimplifierTool tool) {
+        if (!tool.canonicalizeReads() || !canCanonicalizeRead()) {
+            return;
+        }
+        if (address instanceof OffsetAddressNode) {
+            OffsetAddressNode objAddress = (OffsetAddressNode) address;
+            ConstantReflectionProvider constantReflection = tool.getConstantReflection();
+            // Note: readConstant cannot be used to read the array length, so in order to avoid an
+            // unnecessary CompilerToVM.readFieldValue call ending in an IllegalArgumentException,
+            // check if we are reading the array length location first.
+            if (getLocationIdentity().equals(ARRAY_LENGTH_LOCATION)) {
+                ValueNode length = GraphUtil.arrayLength(objAddress.getBase(), ArrayLengthProvider.FindLengthMode.CANONICALIZE_READ, constantReflection);
+                if (length != null) {
+                    StructuredGraph graph = graph();
+                    ValueNode replacement = length;
+                    if (!length.isConstant() && length.stamp(NodeView.DEFAULT).canBeImprovedWith(StampFactory.positiveInt())) {
+                        ValueAnchorNode g = graph.add(new ValueAnchorNode());
+                        graph.addAfterFixed(this, g);
+                        replacement = graph.addWithoutUnique(new PiNode(length, StampFactory.positiveInt(), g));
+                    }
+                    if (!replacement.isAlive()) {
+                        replacement = graph.addOrUnique(replacement);
+                    }
+                    graph.replaceFixedWithFloating(this, replacement);
+                }
+            }
+        }
+    }
+
     @Override
     public LocationIdentity getKilledLocationIdentity() {
         if (ordersMemoryAccesses()) {
@@ -204,7 +244,8 @@ private static ValueNode canonicalizeRead(ValueNode read, Stamp accessStamp, Val
         // check if we are reading the array length location first.
         if (locationIdentity.equals(ARRAY_LENGTH_LOCATION)) {
             ValueNode length = GraphUtil.arrayLength(object, ArrayLengthProvider.FindLengthMode.CANONICALIZE_READ, constantReflection);
-            if (length != null) {
+            // only use length if we do not drop stamp precision here
+            if (length != null && !length.stamp(NodeView.DEFAULT).canBeImprovedWith(StampFactory.positiveInt())) {
                 assert length.stamp(view).isCompatible(accessStamp);
                 return length;
             }

diff --git a/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/phases/common/LoweringPhase.java b/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/phases/common/LoweringPhase.java
@@ -117,6 +117,8 @@ public static class Options {
         //@formatter:off
         @Option(help = "Print schedule result pre lowering to TTY.", type = OptionType.Expert)
         public static final OptionKey<Boolean> PrintLoweringScheduleToTTY = new OptionKey<>(false);
+        @Option(help = "Dump lowering after every node to igv.", type = OptionType.Expert)
+        public static final OptionKey<Boolean> DumpAfterEveryLowering = new OptionKey<>(false);
         //@formatter:on
     }
 
@@ -584,7 +586,6 @@ public void postprocess() {
 
     @SuppressWarnings("try")
     private AnchoringNode process(CoreProviders context, final HIRBlock b, final NodeBitMap activeGuards, final AnchoringNode startAnchor, ScheduleResult schedule) {
-
         FixedWithNextNode lastFixedNode = b.getBeginNode();
         if (b.getBeginNode() instanceof LoopExitNode) {
             /**
@@ -693,6 +694,9 @@ private AnchoringNode process(CoreProviders context, final HIRBlock b, final Nod
                 }
                 loweringTool.setLastFixedNode((FixedWithNextNode) nextLastFixed);
             }
+            if (Options.DumpAfterEveryLowering.getValue(debug.getOptions())) {
+                debug.dump(DebugContext.VERY_DETAILED_LEVEL, b.getBeginNode().graph(), "After lowering %s", node);
+            }
         }
         return loweringTool.getCurrentGuardAnchor();
     }

diff --git a/...c/jdk.graal.compiler/src/jdk/graal/compiler/replacements/DefaultJavaLoweringProvider.java b/...c/jdk.graal.compiler/src/jdk/graal/compiler/replacements/DefaultJavaLoweringProvider.java
@@ -625,8 +625,8 @@ public void lowerStoreIndexedNode(StoreIndexedNode storeIndexed, LoweringTool to
     }
 
     protected void lowerArrayLengthNode(ArrayLengthNode arrayLengthNode, LoweringTool tool) {
-        arrayLengthNode.replaceAtUsages(createReadArrayLength(arrayLengthNode.array(), arrayLengthNode, tool));
         StructuredGraph graph = arrayLengthNode.graph();
+        arrayLengthNode.replaceAtUsages(createReadArrayLength(arrayLengthNode.array(), arrayLengthNode, tool));
         graph.removeFixed(arrayLengthNode);
     }
 
@@ -635,7 +635,7 @@ protected void lowerArrayLengthNode(ArrayLengthNode arrayLengthNode, LoweringToo
      *
      * The created node is placed before {@code before} in the CFG.
      */
-    protected ReadNode createReadArrayLength(ValueNode array, FixedNode before, LoweringTool tool) {
+    private ReadNode createReadArrayLength(ValueNode array, FixedNode before, LoweringTool tool) {
         StructuredGraph graph = array.graph();
         ValueNode canonicalArray = this.createNullCheckedValue(GraphUtil.skipPiWhileNonNullArray(array), before, tool);
         AddressNode address = createOffsetAddress(graph, canonicalArray, arrayLengthOffset());