# HG changeset patch # User Doug Simon # Date 1373032128 -7200 # Node ID ba1fbbfac0cd1e2f977e81f58f4ad4fd5e99baeb # Parent bef82f0cf71dab35ccfe0d8597b4026a5acbbc54 remove null check semantics from LoadHubNode (GRAAL-248) diff -r bef82f0cf71d -r ba1fbbfac0cd graal/com.oracle.graal.hotspot/src/com/oracle/graal/hotspot/meta/HotSpotRuntime.java --- a/graal/com.oracle.graal.hotspot/src/com/oracle/graal/hotspot/meta/HotSpotRuntime.java Fri Jul 05 15:47:31 2013 +0200 +++ b/graal/com.oracle.graal.hotspot/src/com/oracle/graal/hotspot/meta/HotSpotRuntime.java Fri Jul 05 15:48:48 2013 +0200 @@ -490,11 +490,13 @@ } else if (n instanceof Invoke) { Invoke invoke = (Invoke) n; if (invoke.callTarget() instanceof MethodCallTargetNode) { + MethodCallTargetNode callTarget = (MethodCallTargetNode) invoke.callTarget(); NodeInputList parameters = callTarget.arguments(); ValueNode receiver = parameters.size() <= 0 ? null : parameters.get(0); + GuardingNode receiverNullCheck = null; if (!callTarget.isStatic() && receiver.kind() == Kind.Object && !receiver.objectStamp().nonNull()) { - tool.createNullCheckGuard(invoke, receiver); + receiverNullCheck = tool.createNullCheckGuard(invoke, receiver); } JavaType[] signature = MetaUtil.signatureToTypes(callTarget.targetMethod().getSignature(), callTarget.isStatic() ? null : callTarget.targetMethod().getDeclaringClass()); @@ -506,7 +508,12 @@ if (hsMethod.isInVirtualMethodTable()) { int vtableEntryOffset = hsMethod.vtableEntryOffset(); assert vtableEntryOffset > 0; - ReadNode hub = this.createReadHub(tool, graph, wordKind, receiver); + ReadNode hub = createReadHub(graph, wordKind, receiver); + + if (receiverNullCheck != null) { + hub.setGuard(receiverNullCheck); + } + ReadNode metaspaceMethod = createReadVirtualMethod(graph, wordKind, hub, hsMethod); // We use LocationNode.ANY_LOCATION for the reads that access the // compiled code entry as HotSpot does not guarantee they are final @@ -631,7 +638,10 @@ LoadHubNode loadHub = (LoadHubNode) n; assert loadHub.kind() == wordKind; ValueNode object = loadHub.object(); - ReadNode hub = createReadHub(tool, graph, wordKind, object); + ReadNode hub = createReadHub(graph, wordKind, object); + // A hub read must not float outside its block otherwise + // it may float above an explicit null check on its object. + hub.setGuard(AbstractBeginNode.prevBegin(loadHub)); graph.replaceFixed(loadHub, hub); } else if (n instanceof LoadMethodNode) { LoadMethodNode loadMethodNode = (LoadMethodNode) n; @@ -822,12 +832,10 @@ return metaspaceMethod; } - private ReadNode createReadHub(LoweringTool tool, StructuredGraph graph, Kind wordKind, ValueNode object) { + private ReadNode createReadHub(StructuredGraph graph, Kind wordKind, ValueNode object) { LocationNode location = ConstantLocationNode.create(FINAL_LOCATION, wordKind, config.hubOffset, graph); assert !object.isConstant() || object.asConstant().isNull(); - ReadNode hub = graph.add(new ReadNode(object, location, StampFactory.forKind(wordKind()), WriteBarrierType.NONE, false)); - tool.createNullCheckGuard(hub, object); - return hub; + return graph.add(new ReadNode(object, location, StampFactory.forKind(wordKind()), WriteBarrierType.NONE, false)); } public static long referentOffset() { @@ -914,11 +922,11 @@ ifNodeOffset.replaceAtUsages(phiNode); } else { IndexedLocationNode location = IndexedLocationNode.create(ANY_LOCATION, load.accessKind(), load.displacement(), load.offset(), graph, 1); - // Unsafe Accesses to the metaspace or to any + // Unsafe access to a metaspace or to any // absolute address do not perform uncompression. ReadNode memoryRead = graph.add(new ReadNode(load.object(), location, load.stamp(), WriteBarrierType.NONE, compress)); - // An unsafe read must not floating outside its block as may float above an explicit - // null check on its object. + // An unsafe read must not float outside its block otherwise + // it may float above an explicit null check on its object. memoryRead.setGuard(AbstractBeginNode.prevBegin(load)); graph.replaceFixedWithFixed(load, memoryRead); } diff -r bef82f0cf71d -r ba1fbbfac0cd graal/com.oracle.graal.hotspot/src/com/oracle/graal/hotspot/replacements/ArrayCopySnippets.java --- a/graal/com.oracle.graal.hotspot/src/com/oracle/graal/hotspot/replacements/ArrayCopySnippets.java Fri Jul 05 15:47:31 2013 +0200 +++ b/graal/com.oracle.graal.hotspot/src/com/oracle/graal/hotspot/replacements/ArrayCopySnippets.java Fri Jul 05 15:48:48 2013 +0200 @@ -24,6 +24,8 @@ import static com.oracle.graal.api.meta.LocationIdentity.*; import static com.oracle.graal.hotspot.replacements.HotSpotReplacementsUtil.*; +import static com.oracle.graal.nodes.GuardingPiNode.*; +import static com.oracle.graal.nodes.calc.IsNullNode.*; import static com.oracle.graal.phases.GraalOptions.*; import static com.oracle.graal.replacements.nodes.BranchProbabilityNode.*; @@ -35,10 +37,13 @@ import com.oracle.graal.graph.*; import com.oracle.graal.hotspot.nodes.*; import com.oracle.graal.nodes.*; +import com.oracle.graal.nodes.calc.*; import com.oracle.graal.nodes.extended.*; import com.oracle.graal.nodes.java.*; +import com.oracle.graal.nodes.type.*; import com.oracle.graal.phases.*; import com.oracle.graal.replacements.*; +import com.oracle.graal.replacements.Snippet.Fold; import com.oracle.graal.replacements.nodes.*; import com.oracle.graal.word.*; @@ -77,17 +82,19 @@ private static final long VECTOR_SIZE = arrayIndexScale(Kind.Long); private static void checkedCopy(Object src, int srcPos, Object dest, int destPos, int length, Kind baseKind) { - checkNonNull(src); - checkNonNull(dest); - checkLimits(src, srcPos, dest, destPos, length); - UnsafeArrayCopyNode.arraycopy(src, srcPos, dest, destPos, length, baseKind); + Object nonNullSrc = checkNonNull(src); + Object nonNullDest = checkNonNull(dest); + checkLimits(nonNullSrc, srcPos, nonNullDest, destPos, length); + UnsafeArrayCopyNode.arraycopy(nonNullSrc, srcPos, nonNullDest, destPos, length, baseKind); } - public static void checkNonNull(Object obj) { - if (obj == null) { - checkNPECounter.inc(); - DeoptimizeNode.deopt(DeoptimizationAction.None, DeoptimizationReason.RuntimeConstraint); - } + @Fold + private static Stamp nonNull() { + return StampFactory.objectNonNull(); + } + + public static Object checkNonNull(Object obj) { + return guardingPi(obj, isNull(obj), true, DeoptimizationReason.RuntimeConstraint, DeoptimizationAction.None, nonNull()); } public static int checkArrayType(Word hub) { @@ -184,24 +191,25 @@ @Snippet public static void arraycopy(Object src, int srcPos, Object dest, int destPos, int length) { - // loading the hubs also checks for nullness - Word srcHub = loadHub(src); - Word destHub = loadHub(dest); - if (probability(FAST_PATH_PROBABILITY, srcHub.equal(destHub)) && probability(FAST_PATH_PROBABILITY, src != dest)) { + Object nonNullSrc = checkNonNull(src); + Object nonNullDest = checkNonNull(dest); + Word srcHub = loadHub(nonNullSrc); + Word destHub = loadHub(nonNullDest); + if (probability(FAST_PATH_PROBABILITY, srcHub.equal(destHub)) && probability(FAST_PATH_PROBABILITY, nonNullSrc != nonNullDest)) { int layoutHelper = checkArrayType(srcHub); final boolean isObjectArray = ((layoutHelper & layoutHelperElementTypePrimitiveInPlace()) == 0); - checkLimits(src, srcPos, dest, destPos, length); + checkLimits(nonNullSrc, srcPos, nonNullDest, destPos, length); if (probability(FAST_PATH_PROBABILITY, isObjectArray)) { genericObjectExactCallCounter.inc(); - arrayObjectCopy(src, srcPos, dest, destPos, length); + arrayObjectCopy(nonNullSrc, srcPos, nonNullDest, destPos, length); } else { genericPrimitiveCallCounter.inc(); - UnsafeArrayCopyNode.arraycopyPrimitive(src, srcPos, dest, destPos, length, layoutHelper); + UnsafeArrayCopyNode.arraycopyPrimitive(nonNullSrc, srcPos, nonNullDest, destPos, length, layoutHelper); } } else { genericObjectCallCounter.inc(); - System.arraycopy(src, srcPos, dest, destPos, length); + System.arraycopy(nonNullSrc, srcPos, nonNullDest, destPos, length); } } diff -r bef82f0cf71d -r ba1fbbfac0cd graal/com.oracle.graal.hotspot/src/com/oracle/graal/hotspot/replacements/HotSpotReplacementsUtil.java --- a/graal/com.oracle.graal.hotspot/src/com/oracle/graal/hotspot/replacements/HotSpotReplacementsUtil.java Fri Jul 05 15:47:31 2013 +0200 +++ b/graal/com.oracle.graal.hotspot/src/com/oracle/graal/hotspot/replacements/HotSpotReplacementsUtil.java Fri Jul 05 15:48:48 2013 +0200 @@ -437,19 +437,12 @@ } /** - * Loads the hub from a object, null checking it first. + * Loads the hub of an object (without null checking it first). */ public static Word loadHub(Object object) { return loadHubIntrinsic(object, getWordKind()); } - /** - * Loads the hub from a object. - */ - public static Word loadHubNoNullcheck(Object object) { - return loadWordFromObject(object, hubOffset()); - } - public static Object verifyOop(Object object) { if (verifyOops()) { verifyOopStub(VERIFY_OOP, object); diff -r bef82f0cf71d -r ba1fbbfac0cd graal/com.oracle.graal.hotspot/src/com/oracle/graal/hotspot/replacements/MonitorSnippets.java --- a/graal/com.oracle.graal.hotspot/src/com/oracle/graal/hotspot/replacements/MonitorSnippets.java Fri Jul 05 15:47:31 2013 +0200 +++ b/graal/com.oracle.graal.hotspot/src/com/oracle/graal/hotspot/replacements/MonitorSnippets.java Fri Jul 05 15:48:48 2013 +0200 @@ -107,7 +107,7 @@ } else { // The bias pattern is present in the object's mark word. Need to check // whether the bias owner and the epoch are both still current. - Word hub = loadHubNoNullcheck(object); + Word hub = loadHub(object); final Word prototypeMarkWord = hub.readWord(prototypeMarkWordOffset(), PROTOTYPE_MARK_WORD_LOCATION); final Word thread = thread(); final Word tmp = prototypeMarkWord.or(thread).xor(mark).and(~ageMaskInPlace()); diff -r bef82f0cf71d -r ba1fbbfac0cd graal/com.oracle.graal.nodes/src/com/oracle/graal/nodes/calc/IsNullNode.java --- a/graal/com.oracle.graal.nodes/src/com/oracle/graal/nodes/calc/IsNullNode.java Fri Jul 05 15:47:31 2013 +0200 +++ b/graal/com.oracle.graal.nodes/src/com/oracle/graal/nodes/calc/IsNullNode.java Fri Jul 05 15:48:48 2013 +0200 @@ -92,4 +92,7 @@ } return false; } + + @NodeIntrinsic + public static native IsNullNode isNull(Object object); } diff -r bef82f0cf71d -r ba1fbbfac0cd graal/com.oracle.graal.nodes/src/com/oracle/graal/nodes/extended/LoadHubNode.java --- a/graal/com.oracle.graal.nodes/src/com/oracle/graal/nodes/extended/LoadHubNode.java Fri Jul 05 15:47:31 2013 +0200 +++ b/graal/com.oracle.graal.nodes/src/com/oracle/graal/nodes/extended/LoadHubNode.java Fri Jul 05 15:48:48 2013 +0200 @@ -29,7 +29,8 @@ import com.oracle.graal.nodes.type.*; /** - * Loads an object's {@linkplain Representation#ObjectHub hub}, null-checking the object first. + * Loads an object's {@linkplain Representation#ObjectHub hub}. The object is not null-checked by + * this operation. */ public final class LoadHubNode extends FixedWithNextNode implements Lowerable, Canonicalizable, Virtualizable { diff -r bef82f0cf71d -r ba1fbbfac0cd graal/com.oracle.graal.phases.common/src/com/oracle/graal/phases/common/InliningUtil.java --- a/graal/com.oracle.graal.phases.common/src/com/oracle/graal/phases/common/InliningUtil.java Fri Jul 05 15:47:31 2013 +0200 +++ b/graal/com.oracle.graal.phases.common/src/com/oracle/graal/phases/common/InliningUtil.java Fri Jul 05 15:48:48 2013 +0200 @@ -22,6 +22,9 @@ */ package com.oracle.graal.phases.common; +import static com.oracle.graal.api.code.DeoptimizationAction.*; +import static com.oracle.graal.api.meta.DeoptimizationReason.*; +import static com.oracle.graal.nodes.type.StampFactory.*; import static com.oracle.graal.phases.GraalOptions.*; import java.lang.reflect.*; @@ -251,18 +254,26 @@ } /** - * Represents an opportunity for inlining at the given invoke, with the given weight and level. + * Represents an opportunity for inlining at a given invoke, with the given weight and level. * The weight is the amortized weight of the additional code - so smaller is better. The level * is the number of nested inlinings that lead to this invoke. */ public interface InlineInfo { + /** + * The graph containing the {@link #invoke() invocation} that may be inlined. + */ StructuredGraph graph(); + /** + * The invocation that may be inlined. + */ Invoke invoke(); /** - * Returns the number of invoked methods. + * Returns the number of methods that may be inlined by the {@link #invoke() invocation}. + * This may be more than one in the case of a invocation profile showing a number of "hot" + * concrete methods dispatched to by the invocation. */ int numberOfMethods(); @@ -485,16 +496,16 @@ } private void createGuard(StructuredGraph graph, MetaAccessProvider runtime) { - InliningUtil.receiverNullCheck(invoke); - ValueNode receiver = ((MethodCallTargetNode) invoke.callTarget()).receiver(); + ValueNode nonNullReceiver = InliningUtil.nonNullReceiver(invoke); ConstantNode typeHub = ConstantNode.forConstant(type.getEncoding(Representation.ObjectHub), runtime, graph); - LoadHubNode receiverHub = graph.add(new LoadHubNode(receiver, typeHub.kind())); + LoadHubNode receiverHub = graph.add(new LoadHubNode(nonNullReceiver, typeHub.kind())); + CompareNode typeCheck = CompareNode.createCompareNode(Condition.EQ, receiverHub, typeHub); FixedGuardNode guard = graph.add(new FixedGuardNode(typeCheck, DeoptimizationReason.TypeCheckedInliningViolated, DeoptimizationAction.InvalidateReprofile)); assert invoke.predecessor() != null; - ValueNode anchoredReceiver = createAnchoredReceiver(graph, guard, type, receiver, true); - invoke.callTarget().replaceFirstInput(receiver, anchoredReceiver); + ValueNode anchoredReceiver = createAnchoredReceiver(graph, guard, type, nonNullReceiver, true); + invoke.callTarget().replaceFirstInput(nonNullReceiver, anchoredReceiver); graph.addBeforeFixed(invoke.asNode(), receiverHub); graph.addBeforeFixed(invoke.asNode(), guard); @@ -508,7 +519,7 @@ /** * Polymorphic inlining of m methods with n type checks (n >= m) in case that the profiling - * information suggests a reasonable amounts of different receiver types and different methods. + * information suggests a reasonable amount of different receiver types and different methods. * If an unknown type is encountered a deoptimization is triggered. */ private static class MultiTypeGuardInlineInfo extends AbstractInlineInfo { @@ -592,8 +603,6 @@ @Override public void inline(MetaAccessProvider runtime, Assumptions assumptions, Replacements replacements) { - // receiver null check must be the first node - InliningUtil.receiverNullCheck(invoke); if (hasSingleMethod()) { inlineSingleMethod(graph(), runtime, assumptions); } else { @@ -773,9 +782,9 @@ private boolean createDispatchOnTypeBeforeInvoke(StructuredGraph graph, AbstractBeginNode[] successors, boolean invokeIsOnlySuccessor, MetaAccessProvider runtime) { assert ptypes.size() >= 1; - + ValueNode nonNullReceiver = nonNullReceiver(invoke); Kind hubKind = ((MethodCallTargetNode) invoke.callTarget()).targetMethod().getDeclaringClass().getEncoding(Representation.ObjectHub).getKind(); - LoadHubNode hub = graph.add(new LoadHubNode(((MethodCallTargetNode) invoke.callTarget()).receiver(), hubKind)); + LoadHubNode hub = graph.add(new LoadHubNode(nonNullReceiver, hubKind)); graph.addBeforeFixed(invoke.asNode(), hub); if (!invokeIsOnlySuccessor && chooseMethodDispatch()) { @@ -960,8 +969,6 @@ } private void devirtualizeWithTypeSwitch(StructuredGraph graph, InvokeKind kind, ResolvedJavaMethod target, MetaAccessProvider runtime) { - InliningUtil.receiverNullCheck(invoke); - AbstractBeginNode invocationEntry = graph.add(new BeginNode()); AbstractBeginNode unknownTypeSux = createUnknownTypeSuccessor(graph); AbstractBeginNode[] successors = new AbstractBeginNode[]{invocationEntry, unknownTypeSux}; @@ -1052,6 +1059,10 @@ ResolvedJavaType holder = targetMethod.getDeclaringClass(); ObjectStamp receiverStamp = callTarget.receiver().objectStamp(); + if (receiverStamp.alwaysNull()) { + // Don't inline if receiver is known to be null + return null; + } if (receiverStamp.type() != null) { // the invoke target might be more specific than the holder (happens after inlining: // locals lose their declared type...) @@ -1300,17 +1311,8 @@ FrameState stateAfter = invoke.stateAfter(); assert stateAfter == null || stateAfter.isAlive(); - if (receiverNullCheck) { - GuardingNode receiverNullCheckNode = receiverNullCheck(invoke); - if (receiverNullCheckNode != null) { - ValueNode receiver = invoke.callTarget().arguments().get(0); - Stamp piStamp = receiver.stamp(); - if (piStamp instanceof ObjectStamp) { - piStamp = piStamp.join(StampFactory.objectNonNull()); - } - ValueNode anchoredReceiver = createAnchoredReceiver(graph, receiverNullCheckNode, receiver, piStamp); - invoke.callTarget().replaceFirstInput(receiver, anchoredReceiver); - } + if (receiverNullCheck && !((MethodCallTargetNode) invoke.callTarget()).isStatic()) { + nonNullReceiver(invoke); } IdentityHashMap replacements = new IdentityHashMap<>(); @@ -1448,17 +1450,23 @@ return true; } - public static GuardingNode receiverNullCheck(Invoke invoke) { + /** + * Gets the receiver for an invoke, adding a guard if necessary to ensure it is non-null. + */ + public static ValueNode nonNullReceiver(Invoke invoke) { MethodCallTargetNode callTarget = (MethodCallTargetNode) invoke.callTarget(); + assert !callTarget.isStatic() : callTarget.targetMethod(); StructuredGraph graph = callTarget.graph(); - NodeInputList parameters = callTarget.arguments(); - ValueNode firstParam = parameters.size() <= 0 ? null : parameters.get(0); - if (!callTarget.isStatic() && firstParam.kind() == Kind.Object && !firstParam.objectStamp().nonNull()) { - FixedGuardNode guard = graph.add(new FixedGuardNode(graph.unique(new IsNullNode(firstParam)), DeoptimizationReason.NullCheckException, DeoptimizationAction.InvalidateReprofile, true)); - graph.addBeforeFixed(invoke.asNode(), guard); - return guard; + ValueNode firstParam = callTarget.arguments().get(0); + if (firstParam.kind() == Kind.Object && !firstParam.objectStamp().nonNull()) { + assert !firstParam.objectStamp().alwaysNull(); + IsNullNode condition = graph.unique(new IsNullNode(firstParam)); + GuardingPiNode nonNullReceiver = graph.add(new GuardingPiNode(firstParam, condition, true, NullCheckException, InvalidateReprofile, objectNonNull())); + graph.addBeforeFixed(invoke.asNode(), nonNullReceiver); + callTarget.replaceFirstInput(firstParam, nonNullReceiver); + return nonNullReceiver; } - return null; + return firstParam; } public static boolean canIntrinsify(Replacements replacements, ResolvedJavaMethod target) {