From ad2a494da02cddf752f75f143d8a9f3e4cfb2ea5 Mon Sep 17 00:00:00 2001 From: Carlo Refice Date: Wed, 25 Oct 2023 12:37:34 +0200 Subject: [PATCH 1/2] Improve canonicalization of InstanceOfNode When the intersection between an InstanceOfNode's input stamp and checked stamp is null, the check will only succeed if the input is null, and can thus be simplified to a null check. --- compiler/mx.compiler/mx_compiler.py | 12 ++++----- .../compiler/nodes/java/InstanceOfNode.java | 25 +++++++++---------- 2 files changed, 18 insertions(+), 19 deletions(-) diff --git a/compiler/mx.compiler/mx_compiler.py b/compiler/mx.compiler/mx_compiler.py index c313b6351669..87d204f0d6f9 100644 --- a/compiler/mx.compiler/mx_compiler.py +++ b/compiler/mx.compiler/mx_compiler.py @@ -566,14 +566,14 @@ def compiler_gate_benchmark_runner(tasks, extraVMarguments=None, prefix='', task if t: for name in dacapo_suite.benchmarkList(bmSuiteArgs): iterations = int(dacapo_suite.daCapoIterations().get(name, -1) * default_iterations_reduction) - for i in range(default_iterations * scala_daily_scaling_factor): + for _ in range(default_iterations * scala_daily_scaling_factor): _gate_dacapo(name, iterations, benchVmArgs + ['-Dgraal.TrackNodeSourcePosition=true'] + dacapo_esa) with mx_gate.Task('Dacapo benchmark weekly workload', tasks, tags=['dacapo_weekly'], report=task_report_component) as t: if t: for name in dacapo_suite.benchmarkList(bmSuiteArgs): iterations = int(dacapo_suite.daCapoIterations().get(name, -1) * default_iterations_reduction) - for i in range(default_iterations * scala_weekly_scaling_factor): + for _ in range(default_iterations * scala_weekly_scaling_factor): _gate_dacapo(name, iterations, benchVmArgs + ['-Dgraal.TrackNodeSourcePosition=true'] + dacapo_esa) # ensure we can also run on C2 @@ -603,14 +603,14 @@ def compiler_gate_benchmark_runner(tasks, extraVMarguments=None, prefix='', task if t: for name in scala_dacapo_suite.benchmarkList(bmSuiteArgs): iterations = int(scala_dacapo_suite.daCapoIterations().get(name, -1) * default_iterations_reduction) - for i in range(default_iterations * scala_dacapo_daily_scaling_factor): + for _ in range(default_iterations * scala_dacapo_daily_scaling_factor): _gate_scala_dacapo(name, iterations, benchVmArgs + ['-Dgraal.TrackNodeSourcePosition=true'] + dacapo_esa) with mx_gate.Task('ScalaDacapo benchmark weekly workload', tasks, tags=['scala_dacapo_weekly'], report=task_report_component) as t: if t: for name in scala_dacapo_suite.benchmarkList(bmSuiteArgs): iterations = int(scala_dacapo_suite.daCapoIterations().get(name, -1) * default_iterations_reduction) - for i in range(default_iterations * scala_dacapo_weekly_scaling_factor): + for _ in range(default_iterations * scala_dacapo_weekly_scaling_factor): _gate_scala_dacapo(name, iterations, benchVmArgs + ['-Dgraal.TrackNodeSourcePosition=true'] + dacapo_esa) # run Renaissance benchmarks # @@ -630,14 +630,14 @@ def compiler_gate_benchmark_runner(tasks, extraVMarguments=None, prefix='', task if t: for name in renaissance_suite.benchmarkList(bmSuiteArgs): iterations = int(renaissance_suite.renaissanceIterations().get(name, -1) * default_iterations_reduction) - for i in range(default_iterations): + for _ in range(default_iterations): _gate_renaissance(name, iterations, benchVmArgs + ['-Dgraal.TrackNodeSourcePosition=true'] + enable_assertions) with mx_gate.Task('Renaissance benchmark weekly workload', tasks, tags=['renaissance_weekly'], report=task_report_component) as t: if t: for name in renaissance_suite.benchmarkList(bmSuiteArgs): iterations = int(renaissance_suite.renaissanceIterations().get(name, -1) * default_iterations_reduction) - for i in range(default_iterations * daily_weekly_jobs_ratio): + for _ in range(default_iterations * daily_weekly_jobs_ratio): _gate_renaissance(name, iterations, benchVmArgs + ['-Dgraal.TrackNodeSourcePosition=true'] + enable_assertions) # run benchmark with non default setup # 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 395ba8fdb791..28d6e9e5cce6 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 @@ -50,7 +50,6 @@ import jdk.graal.compiler.nodes.spi.CanonicalizerTool; import jdk.graal.compiler.nodes.spi.Lowerable; import jdk.graal.compiler.nodes.type.StampTool; - import jdk.vm.ci.meta.JavaKind; import jdk.vm.ci.meta.JavaTypeProfile; import jdk.vm.ci.meta.ResolvedJavaType; @@ -138,17 +137,19 @@ public static LogicNode findSynonym(ObjectStamp checkedStamp, ValueNode object, if (joinedStamp.isEmpty()) { // The check can never succeed, the intersection of the two stamps is empty. return LogicConstantNode.contradiction(); + } else if (joinedStamp.equals(inputStamp)) { + // The check will always succeed, the intersection of the two stamps is equal to the + // input stamp. + return LogicConstantNode.tautology(); + } else if (joinedStamp.alwaysNull()) { + // The intersection of the two stamps is always null => simplify the check. + return IsNullNode.create(object); } else { ObjectStamp meetStamp = (ObjectStamp) checkedStamp.meet(inputStamp); - if (checkedStamp.equals(meetStamp)) { - // The check will always succeed, the union of the two stamps is equal to the - // checked stamp. - return LogicConstantNode.tautology(); - } else if (checkedStamp.alwaysNull()) { - return IsNullNode.create(object); - } else 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()) { assert checkedStamp.nonNull() != inputStamp.nonNull(); - // The only difference makes the null-ness of the value => simplify the check. + // The only difference between the two stamps is their null-ness => simplify the + // check. if (checkedStamp.nonNull()) { return LogicNegationNode.create(IsNullNode.create(object)); } else { @@ -182,8 +183,7 @@ public Stamp getSucceedingStampForValue(boolean negated) { @Override public TriState tryFold(Stamp valueStamp) { - if (valueStamp instanceof ObjectStamp) { - ObjectStamp inputStamp = (ObjectStamp) valueStamp; + if (valueStamp instanceof ObjectStamp inputStamp) { ObjectStamp joinedStamp = (ObjectStamp) checkedStamp.join(inputStamp); if (joinedStamp.isEmpty()) { @@ -232,8 +232,7 @@ public static boolean intrinsify(GraphBuilderContext b, ResolvedJavaType type, V @Override public TriState implies(boolean thisNegated, LogicNode other) { - if (other instanceof InstanceOfNode) { - InstanceOfNode instanceOfNode = (InstanceOfNode) other; + if (other instanceof InstanceOfNode instanceOfNode) { if (instanceOfNode.getValue() == getValue()) { if (thisNegated) { // !X => Y From 65977d2b04a0372a0731adb1326192a79020bdc0 Mon Sep 17 00:00:00 2001 From: Carlo Refice Date: Thu, 26 Oct 2023 11:09:53 +0200 Subject: [PATCH 2/2] Add test for improved InstanceOfNode canonicalization --- .../test/InstanceOfCanonicalizationTest.java | 77 +++++++++++++++++++ 1 file changed, 77 insertions(+) create mode 100644 compiler/src/jdk.graal.compiler.test/src/jdk/graal/compiler/core/test/InstanceOfCanonicalizationTest.java diff --git a/compiler/src/jdk.graal.compiler.test/src/jdk/graal/compiler/core/test/InstanceOfCanonicalizationTest.java b/compiler/src/jdk.graal.compiler.test/src/jdk/graal/compiler/core/test/InstanceOfCanonicalizationTest.java new file mode 100644 index 000000000000..ea222bdefd64 --- /dev/null +++ b/compiler/src/jdk.graal.compiler.test/src/jdk/graal/compiler/core/test/InstanceOfCanonicalizationTest.java @@ -0,0 +1,77 @@ +/* + * 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.core.test; + +import org.junit.Assert; +import org.junit.Test; + +import jdk.graal.compiler.nodes.StructuredGraph; +import jdk.graal.compiler.nodes.StructuredGraph.AllowAssumptions; +import jdk.graal.compiler.nodes.calc.IsNullNode; +import jdk.graal.compiler.nodes.java.InstanceOfNode; + +public class InstanceOfCanonicalizationTest extends GraalCompilerTest { + + public static boolean checkCastIncompatibleTypes(Object arr) { + // Cast first to a byte array, then to a boolean array. This only succeeds if arr is null. + byte[] barr = (byte[]) arr; + boolean[] bbarr = (boolean[]) (Object) barr; + return true; + } + + public static int unsatisfiableInstanceOf(byte[] barr) { + // Plain instanceof does not allow null, so this will never succeed. + if ((Object) barr instanceof boolean[]) { + return -1; + } + return 1; + } + + @Test + public void testCheckCastIncompatibleTypes() { + StructuredGraph g = parseEager("checkCastIncompatibleTypes", AllowAssumptions.NO, getInitialOptions()); + createCanonicalizerPhase().apply(g, getDefaultHighTierContext()); + + // The second check-cast against boolean[] should canonicalize to a null check + Assert.assertEquals(1, g.getNodes().filter(InstanceOfNode.class).count()); + Assert.assertEquals(1, g.getNodes().filter(IsNullNode.class).count()); + + testAgainstExpected(g.method(), new Result(checkCastIncompatibleTypes(null), null), null, new Object[]{null}); + testAgainstExpected(g.method(), new Result(null, new ClassCastException()), null, new Object[]{new byte[10]}); + testAgainstExpected(g.method(), new Result(null, new ClassCastException()), null, new Object[]{new boolean[10]}); + } + + @Test + public void testUnsatisfiableInstanceOf() { + StructuredGraph g = parseEager("unsatisfiableInstanceOf", AllowAssumptions.NO, getInitialOptions()); + createCanonicalizerPhase().apply(g, getDefaultHighTierContext()); + + // Tested condition can never be true, so it should canonicalize to a constant. + Assert.assertEquals(0, g.getNodes().filter(InstanceOfNode.class).count()); + + testAgainstExpected(g.method(), new Result(unsatisfiableInstanceOf(null), null), null, new Object[]{null}); + testAgainstExpected(g.method(), new Result(unsatisfiableInstanceOf(new byte[10]), null), null, new Object[]{new byte[10]}); + } +}