From 8dcd0230ec7d98c5db4a9f720071d500eff1ce21 Mon Sep 17 00:00:00 2001 From: David Leopoldseder Date: Fri, 26 Jul 2024 15:28:01 +0200 Subject: [PATCH] properly handle array bottom stamps in osr narrowing, such stamps need array checks no instance of ones --- .../compiler/hotspot/test/GraalOSRTest.java | 24 ++++++++ .../replacements/test/InstanceOfTest.java | 14 +++++ .../compiler/core/common/type/Stamp.java | 28 +++++++++ .../phases/OnStackReplacementPhase.java | 60 +++++++++++++++---- .../compiler/nodes/java/InstanceOfNode.java | 5 +- 5 files changed, 118 insertions(+), 13 deletions(-) diff --git a/compiler/src/jdk.graal.compiler.test/src/jdk/graal/compiler/hotspot/test/GraalOSRTest.java b/compiler/src/jdk.graal.compiler.test/src/jdk/graal/compiler/hotspot/test/GraalOSRTest.java index d2953a207a80..c1ac458868a2 100644 --- a/compiler/src/jdk.graal.compiler.test/src/jdk/graal/compiler/hotspot/test/GraalOSRTest.java +++ b/compiler/src/jdk.graal.compiler.test/src/jdk/graal/compiler/hotspot/test/GraalOSRTest.java @@ -266,4 +266,28 @@ private static String normalizedDisassembly(String dis) { Pattern cpRef = Pattern.compile("#\\d+"); return Stream.of(dis.split("\n")).map(line -> cpRef.matcher(line.trim()).replaceAll(mr -> "#__")).collect(Collectors.joining(System.lineSeparator())); } + + static ReturnValue testArrayBottom(byte[] aB, int[] aI, int i) { + Object a = null; + long base = 0; + if (i % 2 == 0) { + a = aB; + base = Unsafe.ARRAY_BYTE_BASE_OFFSET; + } else { + a = aI; + base = Unsafe.ARRAY_INT_BASE_OFFSET; + } + int res = 0; + for (int j = 0; j < 10000; j++) { + res += UNSAFE.getByte(a, base + j); + } + GraalDirectives.sideEffect(res); + + return ReturnValue.SUCCESS; + } + + @Test + public void testOSR07() { + testOSR(getInitialOptions(), "testArrayBottom", null, new byte[100], new int[100], 10); + } } diff --git a/compiler/src/jdk.graal.compiler.test/src/jdk/graal/compiler/replacements/test/InstanceOfTest.java b/compiler/src/jdk.graal.compiler.test/src/jdk/graal/compiler/replacements/test/InstanceOfTest.java index 19a8a7fd6cc0..e84127cb07b7 100644 --- a/compiler/src/jdk.graal.compiler.test/src/jdk/graal/compiler/replacements/test/InstanceOfTest.java +++ b/compiler/src/jdk.graal.compiler.test/src/jdk/graal/compiler/replacements/test/InstanceOfTest.java @@ -30,6 +30,7 @@ import java.util.Map; import java.util.TreeMap; +import jdk.graal.compiler.api.directives.GraalDirectives; import jdk.graal.compiler.debug.DebugContext; import jdk.graal.compiler.nodes.IfNode; import jdk.graal.compiler.nodes.ReturnNode; @@ -538,4 +539,17 @@ static class Depth13 extends Depth12 { static class Depth14 extends Depth12 { } + + public static int iof(Object o) { + if (o instanceof Object[]) { + return 0; + } + GraalDirectives.sideEffect(2); + return 1; + } + + @Test + public void testInstanceOfArray() { + test("iof", new Object()); + } } diff --git a/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/core/common/type/Stamp.java b/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/core/common/type/Stamp.java index 5a2e230fbd62..bc2ac90a40f9 100644 --- a/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/core/common/type/Stamp.java +++ b/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/core/common/type/Stamp.java @@ -174,6 +174,34 @@ public boolean isNonObjectPointerStamp() { return isPointerStamp() && !isObjectStamp(); } + /** + * Tests whether this stamp represents an array type without any additional knowledge about + * nullness or its component type. Normally such stamps are only possible at places where + * control flow merges array types that have their least common ancestor in + * {@link java.lang.Object}. + * + * An example of such a stamp can be seen in the following code + * + * 
+     * void foo(boolean a, int[] iA, byte[] bA) {
+     *     Object o = null;
+     *     if (a) {
+     *         o = iA;
+     *     } else {
+     *         o = bA;
+     *     }
+     *     use(o); // potentially null, always array stamp without a concrete type
+     * }
+     *
+ */ + public boolean isBottomArrayType() { + if (isObjectStamp()) { + ObjectStamp o = ((ObjectStamp) this); + return o.isAlwaysArray() && o.type() == null; + } + return false; + } + /** * If this stamp represents a single value, the methods returns this single value. It returns * null otherwise. diff --git a/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/hotspot/phases/OnStackReplacementPhase.java b/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/hotspot/phases/OnStackReplacementPhase.java index d69adfbe0efb..4335ac27c9f2 100644 --- a/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/hotspot/phases/OnStackReplacementPhase.java +++ b/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/hotspot/phases/OnStackReplacementPhase.java @@ -49,8 +49,10 @@ import jdk.graal.compiler.nodes.EntryProxyNode; import jdk.graal.compiler.nodes.FixedGuardNode; import jdk.graal.compiler.nodes.FixedNode; +import jdk.graal.compiler.nodes.FixedWithNextNode; import jdk.graal.compiler.nodes.FrameState; import jdk.graal.compiler.nodes.GraphState; +import jdk.graal.compiler.nodes.LogicNegationNode; import jdk.graal.compiler.nodes.LogicNode; import jdk.graal.compiler.nodes.LoopBeginNode; import jdk.graal.compiler.nodes.NodeView; @@ -59,11 +61,13 @@ import jdk.graal.compiler.nodes.StartNode; import jdk.graal.compiler.nodes.StructuredGraph; import jdk.graal.compiler.nodes.ValueNode; +import jdk.graal.compiler.nodes.calc.IsNullNode; import jdk.graal.compiler.nodes.cfg.HIRBlock; import jdk.graal.compiler.nodes.extended.OSRLocalNode; import jdk.graal.compiler.nodes.extended.OSRLockNode; import jdk.graal.compiler.nodes.extended.OSRMonitorEnterNode; import jdk.graal.compiler.nodes.extended.OSRStartNode; +import jdk.graal.compiler.nodes.extended.ObjectIsArrayNode; import jdk.graal.compiler.nodes.java.AccessMonitorNode; import jdk.graal.compiler.nodes.java.InstanceOfNode; import jdk.graal.compiler.nodes.java.MonitorEnterNode; @@ -212,20 +216,19 @@ protected void run(StructuredGraph graph, CoreProviders providers) { * sure to infer a more precise one if possible. */ proxy.value().inferStamp(); - Stamp narrowedStamp = proxy.value().stamp(NodeView.DEFAULT); - Stamp unrestrictedStamp = proxy.stamp(NodeView.DEFAULT).unrestricted(); + Stamp proxyValueStamp = proxy.value().stamp(NodeView.DEFAULT); + Stamp proxyUnrestrictedStamp = proxy.stamp(NodeView.DEFAULT).unrestricted(); ValueNode osrLocal; if (i >= localsSize) { - osrLocal = graph.addOrUnique(new OSRLockNode(i - localsSize, unrestrictedStamp)); + osrLocal = graph.addOrUnique(new OSRLockNode(i - localsSize, proxyUnrestrictedStamp)); } else { - osrLocal = initLocal(graph, unrestrictedStamp, oopMap, i); + osrLocal = initLocal(graph, proxyUnrestrictedStamp, oopMap, i); } // Speculate on the OSRLocal stamps that could be more precise. - SpeculationReason reason = OSR_LOCAL_SPECULATIONS.createSpeculationReason(osrState.bci, narrowedStamp, i); - if (graph.getSpeculationLog().maySpeculate(reason) && osrLocal instanceof OSRLocalNode && value.getStackKind().equals(JavaKind.Object) && - !narrowedStamp.isUnrestricted()) { - osrLocal = narrowOsrLocal(graph, narrowedStamp, osrLocal, reason, osrStart, proxy, osrState); + SpeculationReason reason = OSR_LOCAL_SPECULATIONS.createSpeculationReason(osrState.bci, proxyValueStamp, i); + if (graph.getSpeculationLog().maySpeculate(reason) && osrLocal instanceof OSRLocalNode && value.getStackKind().equals(JavaKind.Object) && !proxyValueStamp.isUnrestricted()) { + osrLocal = narrowOsrLocal(graph, proxyValueStamp, osrLocal, reason, osrStart, proxy, osrState); } proxy.replaceAndDelete(osrLocal); @@ -294,17 +297,50 @@ protected void run(StructuredGraph graph, CoreProviders providers) { */ private static ValueNode narrowOsrLocal(StructuredGraph graph, Stamp narrowedStamp, ValueNode osrLocal, SpeculationReason reason, OSRStartNode osrStart, EntryProxyNode proxy, FrameState osrState) { + + ValueNode effectiveOsrLocal = osrLocal; + Stamp checkedStamp = narrowedStamp; + FixedWithNextNode insertionPoint = osrStart; + // Add guard. - LogicNode check = graph.addOrUniqueWithInputs(InstanceOfNode.createHelper((ObjectStamp) narrowedStamp, osrLocal, null, null)); + LogicNode check = null; + + if (checkedStamp.isBottomArrayType()) { + /* + * We are dealing with a bottom array check that has no exact type. We do not need to + * test against a type, it is not an instance of operation but a null check and an + * isArray check + */ + if (checkedStamp.isObjectStamp() && ((ObjectStamp) checkedStamp).nonNull()) { + // without a null check + check = graph.addOrUniqueWithInputs(ObjectIsArrayNode.create(effectiveOsrLocal)); + } else { + // add a preceding null check with the same speculation reason + check = graph.addOrUniqueWithInputs(LogicNegationNode.create(IsNullNode.create(effectiveOsrLocal))); + SpeculationLog.Speculation constant = graph.getSpeculationLog().speculate(reason); + FixedGuardNode guard = graph.add(new FixedGuardNode(check, DeoptimizationReason.OptimizedTypeCheckViolated, DeoptimizationAction.InvalidateRecompile, constant, false)); + graph.addAfterFixed(osrStart, guard); + PiNode nonNullPi = graph.addOrUnique(new PiNode(effectiveOsrLocal, ((ObjectStamp) checkedStamp).asNonNull(), guard)); + + insertionPoint = guard; + + // with a null check + check = graph.addOrUnique(ObjectIsArrayNode.create(nonNullPi)); + } + checkedStamp = new ObjectStamp(null, false, true, false, true); + } else { + check = graph.addOrUniqueWithInputs(InstanceOfNode.createHelper((ObjectStamp) checkedStamp, effectiveOsrLocal, null, null)); + } + SpeculationLog.Speculation constant = graph.getSpeculationLog().speculate(reason); FixedGuardNode guard = graph.add(new FixedGuardNode(check, DeoptimizationReason.OptimizedTypeCheckViolated, DeoptimizationAction.InvalidateRecompile, constant, false)); - graph.addAfterFixed(osrStart, guard); + graph.addAfterFixed(insertionPoint, guard); // Replace with a more specific type at usages. // We know that we are at the root, // so we need to replace the proxy in the state. - proxy.replaceAtMatchingUsages(osrLocal, n -> n == osrState); - return graph.addOrUnique(new PiNode(osrLocal, narrowedStamp, guard)); + proxy.replaceAtMatchingUsages(effectiveOsrLocal, n -> n == osrState); + return graph.addOrUnique(new PiNode(effectiveOsrLocal, checkedStamp, guard)); } /** diff --git a/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/nodes/java/InstanceOfNode.java b/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/nodes/java/InstanceOfNode.java index 7fed96977d25..471b05817ba4 100644 --- a/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/nodes/java/InstanceOfNode.java +++ b/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/nodes/java/InstanceOfNode.java @@ -147,7 +147,10 @@ public static LogicNode findSynonym(ObjectStamp checkedStamp, ValueNode object, return IsNullNode.create(object); } else { ObjectStamp meetStamp = (ObjectStamp) checkedStamp.meet(inputStamp); - if (Objects.equals(checkedStamp.type(), meetStamp.type()) && checkedStamp.isExactType() == meetStamp.isExactType() && checkedStamp.alwaysNull() == meetStamp.alwaysNull()) { + if (Objects.equals(checkedStamp.type(), meetStamp.type()) && + checkedStamp.isExactType() == meetStamp.isExactType() && + checkedStamp.alwaysNull() == meetStamp.alwaysNull() && + checkedStamp.isAlwaysArray() == meetStamp.isAlwaysArray()) { assert checkedStamp.nonNull() != inputStamp.nonNull() : Assertions.errorMessage(checkedStamp, inputStamp, object); // The only difference between the two stamps is their null-ness => simplify the // check.