Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[JDK-8337125] Properly handle array bottom stamps in osr narrowing. #9405

Merged
merged 1 commit into from
Jul 30, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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
*
* <pre>
* 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
* }
* </pre>
*/
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.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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);

Expand Down Expand Up @@ -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));
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down