changeset 21683:c74d3c9b9de7

Use a marker value in second slot of a two-slot value during parsing; improve assertion checking in FrameStateBuilder
author Christian Wimmer <christian.wimmer@oracle.com>
date Tue, 02 Jun 2015 18:25:16 -0700
parents df4579cb9503
children 889b45a0dedd
files graal/com.oracle.graal.graphbuilderconf/src/com/oracle/graal/graphbuilderconf/NodePlugin.java graal/com.oracle.graal.hotspot/src/com/oracle/graal/hotspot/meta/HotSpotNodePlugin.java graal/com.oracle.graal.java/src/com/oracle/graal/java/BytecodeParser.java graal/com.oracle.graal.java/src/com/oracle/graal/java/FrameStateBuilder.java graal/com.oracle.graal.nodes/src/com/oracle/graal/nodes/FrameState.java graal/com.oracle.graal.replacements/src/com/oracle/graal/replacements/WordOperationPlugin.java
diffstat 6 files changed, 187 insertions(+), 45 deletions(-) [+]
line wrap: on
line diff
--- a/graal/com.oracle.graal.graphbuilderconf/src/com/oracle/graal/graphbuilderconf/NodePlugin.java	Wed Jun 03 02:40:53 2015 +0200
+++ b/graal/com.oracle.graal.graphbuilderconf/src/com/oracle/graal/graphbuilderconf/NodePlugin.java	Tue Jun 02 18:25:16 2015 -0700
@@ -152,4 +152,15 @@
     default boolean handleInstanceOf(GraphBuilderContext b, ValueNode object, ResolvedJavaType type, JavaTypeProfile profile) {
         return false;
     }
+
+    /**
+     * If the plugin {@link GraphBuilderContext#push pushes} a value with a different {@link Kind}
+     * than specified by the bytecode, it must override this method and return {@code true}. This
+     * disables assertion checking for value kinds.
+     *
+     * @param b the context
+     */
+    default boolean canChangeStackKind(GraphBuilderContext b) {
+        return false;
+    }
 }
--- a/graal/com.oracle.graal.hotspot/src/com/oracle/graal/hotspot/meta/HotSpotNodePlugin.java	Wed Jun 03 02:40:53 2015 +0200
+++ b/graal/com.oracle.graal.hotspot/src/com/oracle/graal/hotspot/meta/HotSpotNodePlugin.java	Tue Jun 02 18:25:16 2015 -0700
@@ -66,6 +66,14 @@
     }
 
     @Override
+    public boolean canChangeStackKind(GraphBuilderContext b) {
+        if (b.parsingIntrinsic()) {
+            return wordOperationPlugin.canChangeStackKind(b) || nodeIntrinsificationPlugin.canChangeStackKind(b);
+        }
+        return false;
+    }
+
+    @Override
     public FloatingNode interceptParameter(GraphBuilderContext b, int index, Stamp stamp) {
         if (b.parsingIntrinsic()) {
             return wordOperationPlugin.interceptParameter(b, index, stamp);
--- a/graal/com.oracle.graal.java/src/com/oracle/graal/java/BytecodeParser.java	Wed Jun 03 02:40:53 2015 +0200
+++ b/graal/com.oracle.graal.java/src/com/oracle/graal/java/BytecodeParser.java	Tue Jun 02 18:25:16 2015 -0700
@@ -365,6 +365,12 @@
             this.firstInstructionArray = new FixedWithNextNode[blockMap.getBlockCount()];
             this.entryStateArray = new FrameStateBuilder[blockMap.getBlockCount()];
 
+            /*
+             * Configure the assertion checking behavior of the FrameStateBuilder. This needs to be
+             * done only when assertions are enabled, so it is wrapped in an assertion itself.
+             */
+            assert computeKindVerification(startFrameState);
+
             try (Scope s = Debug.scope("LivenessAnalysis")) {
                 int maxLocals = method.getMaxLocals();
                 liveness = LocalLiveness.compute(stream, blockMap.getBlocks(), maxLocals, blockMap.getLoopCount());
@@ -443,6 +449,28 @@
         }
     }
 
+    private boolean computeKindVerification(FrameStateBuilder startFrameState) {
+        if (blockMap.hasJsrBytecodes) {
+            /*
+             * The JSR return address is an int value, but stored using the astore bytecode. Instead
+             * of weakening the kind assertion checking for all methods, we disable it completely
+             * for methods that contain a JSR bytecode.
+             */
+            startFrameState.disableKindVerification();
+        }
+
+        for (NodePlugin plugin : graphBuilderConfig.getPlugins().getNodePlugins()) {
+            if (plugin.canChangeStackKind(this)) {
+                /*
+                 * We have a plugin that can change the kind of values, so no kind assertion
+                 * checking is possible.
+                 */
+                startFrameState.disableKindVerification();
+            }
+        }
+        return true;
+    }
+
     /**
      * Hook for subclasses to modify the graph start instruction or append new instructions to it.
      *
@@ -524,12 +552,23 @@
             ResolvedJavaMethod original = intrinsicContext.getOriginalMethod();
             ValueNode[] locals;
             if (original.getMaxLocals() == frameState.localsSize() || original.isNative()) {
+                locals = new ValueNode[original.getMaxLocals()];
+                for (int i = 0; i < locals.length; i++) {
+                    ValueNode node = frameState.locals[i];
+                    if (node == FrameState.TWO_SLOT_MARKER) {
+                        node = null;
+                    }
+                    locals[i] = node;
+                }
                 locals = frameState.locals;
             } else {
                 locals = new ValueNode[original.getMaxLocals()];
                 int parameterCount = original.getSignature().getParameterCount(!original.isStatic());
                 for (int i = 0; i < parameterCount; i++) {
                     ValueNode param = frameState.locals[i];
+                    if (param == FrameState.TWO_SLOT_MARKER) {
+                        param = null;
+                    }
                     locals[i] = param;
                     assert param == null || param instanceof ParameterNode || param.isConstant();
                 }
@@ -1249,9 +1288,9 @@
         try {
             currentInvokeReturnType = returnType;
             currentInvokeKind = invokeKind;
-            if (tryGenericInvocationPlugin(args, targetMethod)) {
+            if (tryNodePluginForInvocation(args, targetMethod)) {
                 if (TraceParserPlugins.getValue()) {
-                    traceWithContext("used generic invocation plugin for %s", targetMethod.format("%h.%n(%p)"));
+                    traceWithContext("used node plugin for %s", targetMethod.format("%h.%n(%p)"));
                 }
                 return;
             }
@@ -1370,7 +1409,7 @@
         return false;
     }
 
-    private boolean tryGenericInvocationPlugin(ValueNode[] args, ResolvedJavaMethod targetMethod) {
+    private boolean tryNodePluginForInvocation(ValueNode[] args, ResolvedJavaMethod targetMethod) {
         for (NodePlugin plugin : graphBuilderConfig.getPlugins().getNodePlugins()) {
             if (plugin.handleInvoke(this, targetMethod, args)) {
                 return true;
--- a/graal/com.oracle.graal.java/src/com/oracle/graal/java/FrameStateBuilder.java	Wed Jun 03 02:40:53 2015 +0200
+++ b/graal/com.oracle.graal.java/src/com/oracle/graal/java/FrameStateBuilder.java	Tue Jun 02 18:25:16 2015 -0700
@@ -25,6 +25,7 @@
 import static com.oracle.graal.bytecode.Bytecodes.*;
 import static com.oracle.graal.graph.iterators.NodePredicates.*;
 import static com.oracle.graal.java.BytecodeParser.Options.*;
+import static com.oracle.graal.nodes.FrameState.*;
 import static com.oracle.jvmci.common.JVMCIError.*;
 
 import java.util.*;
@@ -54,6 +55,7 @@
     protected final ValueNode[] locals;
     protected final ValueNode[] stack;
     private ValueNode[] lockedObjects;
+    private boolean canVerifyKind;
 
     /**
      * @see BytecodeFrame#rethrowException
@@ -87,6 +89,11 @@
 
         this.monitorIds = EMPTY_MONITOR_ARRAY;
         this.graph = graph;
+        this.canVerifyKind = true;
+    }
+
+    public void disableKindVerification() {
+        canVerifyKind = false;
     }
 
     public void initializeFromArgumentsArray(ValueNode[] arguments) {
@@ -104,7 +111,11 @@
         for (int i = 0; i < max; i++) {
             Kind kind = sig.getParameterKind(i);
             locals[javaIndex] = arguments[index];
-            javaIndex += kind.getSlotCount();
+            javaIndex++;
+            if (kind.needsTwoSlots()) {
+                locals[javaIndex] = TWO_SLOT_MARKER;
+                javaIndex++;
+            }
             index++;
         }
     }
@@ -156,7 +167,11 @@
                 param = new ParameterNode(index, stamp);
             }
             locals[javaIndex] = graph.unique(param);
-            javaIndex += kind.getSlotCount();
+            javaIndex++;
+            if (kind.needsTwoSlots()) {
+                locals[javaIndex] = TWO_SLOT_MARKER;
+                javaIndex++;
+            }
             index++;
         }
     }
@@ -169,6 +184,7 @@
         this.stack = other.stack.clone();
         this.lockedObjects = other.lockedObjects.length == 0 ? other.lockedObjects : other.lockedObjects.clone();
         this.rethrowException = other.rethrowException;
+        this.canVerifyKind = other.canVerifyKind;
 
         assert locals.length == method.getMaxLocals();
         assert stack.length == Math.max(1, method.getMaxStackSize());
@@ -195,11 +211,11 @@
         StringBuilder sb = new StringBuilder();
         sb.append("[locals: [");
         for (int i = 0; i < locals.length; i++) {
-            sb.append(i == 0 ? "" : ",").append(locals[i] == null ? "_" : locals[i].toString(Verbosity.Id));
+            sb.append(i == 0 ? "" : ",").append(locals[i] == null ? "_" : locals[i] == TWO_SLOT_MARKER ? "#" : locals[i].toString(Verbosity.Id));
         }
         sb.append("] stack: [");
         for (int i = 0; i < stackSize; i++) {
-            sb.append(i == 0 ? "" : ",").append(stack[i] == null ? "_" : stack[i].toString(Verbosity.Id));
+            sb.append(i == 0 ? "" : ",").append(stack[i] == null ? "_" : stack[i] == TWO_SLOT_MARKER ? "#" : stack[i].toString(Verbosity.Id));
         }
         sb.append("] locks: [");
         for (int i = 0; i < lockedObjects.length; i++) {
@@ -293,7 +309,8 @@
         for (int i = 0; i < stackSize(); i++) {
             ValueNode x = stack[i];
             ValueNode y = other.stack[i];
-            if (x != y && (x == null || x.isDeleted() || y == null || y.isDeleted() || x.getKind() != y.getKind())) {
+            assert x != null && y != null;
+            if (x != y && (x == TWO_SLOT_MARKER || x.isDeleted() || y == TWO_SLOT_MARKER || y.isDeleted() || x.getKind() != y.getKind())) {
                 return false;
             }
         }
@@ -358,7 +375,7 @@
         if (currentValue == null || currentValue.isDeleted()) {
             return null;
         } else if (block.isPhiAtMerge(currentValue)) {
-            if (otherValue == null || otherValue.isDeleted() || currentValue.getKind() != otherValue.getKind()) {
+            if (otherValue == null || otherValue == TWO_SLOT_MARKER || otherValue.isDeleted() || currentValue.getKind() != otherValue.getKind()) {
                 propagateDelete((ValuePhiNode) currentValue);
                 return null;
             }
@@ -366,7 +383,9 @@
             return currentValue;
         } else if (currentValue != otherValue) {
             assert !(block instanceof LoopBeginNode) : String.format("Phi functions for loop headers are create eagerly for changed locals and all stack slots: %s != %s", currentValue, otherValue);
-            if (otherValue == null || otherValue.isDeleted() || currentValue.getKind() != otherValue.getKind()) {
+            if (currentValue == TWO_SLOT_MARKER || otherValue == TWO_SLOT_MARKER) {
+                return null;
+            } else if (otherValue == null || otherValue.isDeleted() || currentValue.getKind() != otherValue.getKind()) {
                 return null;
             }
             return createValuePhi(currentValue, otherValue, block);
@@ -422,14 +441,14 @@
     public void insertLoopProxies(LoopExitNode loopExit, FrameStateBuilder loopEntryState) {
         for (int i = 0; i < localsSize(); i++) {
             ValueNode value = locals[i];
-            if (value != null && (!loopEntryState.contains(value) || loopExit.loopBegin().isPhiAtMerge(value))) {
+            if (value != null && value != TWO_SLOT_MARKER && (!loopEntryState.contains(value) || loopExit.loopBegin().isPhiAtMerge(value))) {
                 Debug.log(" inserting proxy for %s", value);
                 locals[i] = ProxyNode.forValue(value, loopExit, graph);
             }
         }
         for (int i = 0; i < stackSize(); i++) {
             ValueNode value = stack[i];
-            if (value != null && (!loopEntryState.contains(value) || loopExit.loopBegin().isPhiAtMerge(value))) {
+            if (value != null && value != TWO_SLOT_MARKER && (!loopEntryState.contains(value) || loopExit.loopBegin().isPhiAtMerge(value))) {
                 Debug.log(" inserting proxy for %s", value);
                 stack[i] = ProxyNode.forValue(value, loopExit, graph);
             }
@@ -446,14 +465,14 @@
     public void insertProxies(Function<ValueNode, ValueNode> proxyFunction) {
         for (int i = 0; i < localsSize(); i++) {
             ValueNode value = locals[i];
-            if (value != null) {
+            if (value != null && value != TWO_SLOT_MARKER) {
                 Debug.log(" inserting proxy for %s", value);
                 locals[i] = proxyFunction.apply(value);
             }
         }
         for (int i = 0; i < stackSize(); i++) {
             ValueNode value = stack[i];
-            if (value != null) {
+            if (value != null && value != TWO_SLOT_MARKER) {
                 Debug.log(" inserting proxy for %s", value);
                 stack[i] = proxyFunction.apply(value);
             }
@@ -467,9 +486,9 @@
         }
     }
 
-    private ValuePhiNode createLoopPhi(AbstractMergeNode block, ValueNode value, boolean stampFromValue) {
-        if (value == null) {
-            return null;
+    private ValueNode createLoopPhi(AbstractMergeNode block, ValueNode value, boolean stampFromValue) {
+        if (value == null || value == TWO_SLOT_MARKER) {
+            return value;
         }
         assert !block.isPhiAtMerge(value) : "phi function for this block already created";
 
@@ -552,12 +571,14 @@
         if (liveIn) {
             for (int i = 0; i < locals.length; i++) {
                 if (!liveness.localIsLiveIn(block, i)) {
+                    assert locals[i] != TWO_SLOT_MARKER || locals[i - 1] == null : "Clearing of second slot must have cleared the first slot too";
                     locals[i] = null;
                 }
             }
         } else {
             for (int i = 0; i < locals.length; i++) {
                 if (!liveness.localIsLiveOut(block, i)) {
+                    assert locals[i] != TWO_SLOT_MARKER || locals[i - 1] == null : "Clearing of second slot must have cleared the first slot too";
                     locals[i] = null;
                 }
             }
@@ -603,6 +624,17 @@
         return stackSize;
     }
 
+    private boolean verifyKind(Kind slotKind, ValueNode x) {
+        assert x != null;
+        assert x != TWO_SLOT_MARKER;
+        assert slotKind.getSlotCount() > 0;
+
+        if (canVerifyKind) {
+            assert x.getKind().getStackKind() == slotKind.getStackKind();
+        }
+        return true;
+    }
+
     /**
      * Loads the local variable at the specified index, checking that the returned value is non-null
      * and that two-stack values are properly handled.
@@ -612,11 +644,9 @@
      * @return the instruction that produced the specified local
      */
     public ValueNode loadLocal(int i, Kind slotKind) {
-        assert slotKind.getSlotCount() > 0;
-        assert slotKind.getSlotCount() == 1 || locals[i + 1] == null;
-
         ValueNode x = locals[i];
-        assert x != null : i;
+        assert verifyKind(slotKind, x);
+        assert slotKind.needsTwoSlots() ? locals[i + 1] == TWO_SLOT_MARKER : (i == locals.length - 1 || locals[i + 1] != TWO_SLOT_MARKER);
         return x;
     }
 
@@ -629,10 +659,21 @@
      * @param x the instruction which produces the value for the local
      */
     public void storeLocal(int i, Kind slotKind, ValueNode x) {
-        assert slotKind.getSlotCount() > 0;
+        assert verifyKind(slotKind, x);
 
+        if (locals[i] == TWO_SLOT_MARKER) {
+            /* Writing the second slot of a two-slot value invalidates the first slot. */
+            locals[i - 1] = null;
+        }
         locals[i] = x;
         if (slotKind.needsTwoSlots()) {
+            /* Writing a two-slot value: mark the second slot. */
+            locals[i + 1] = TWO_SLOT_MARKER;
+        } else if (i < locals.length - 1 && locals[i + 1] == TWO_SLOT_MARKER) {
+            /*
+             * Writing a one-slot value to an index previously occupied by a two-slot value: clear
+             * the old marker of the second slot.
+             */
             locals[i + 1] = null;
         }
     }
@@ -644,11 +685,11 @@
      * @param x the instruction to push onto the stack
      */
     public void push(Kind slotKind, ValueNode x) {
-        assert x != null;
-        assert slotKind.getSlotCount() > 0;
+        assert verifyKind(slotKind, x);
+
         xpush(x);
         if (slotKind.needsTwoSlots()) {
-            xpush(null);
+            xpush(TWO_SLOT_MARKER);
         }
     }
 
@@ -665,26 +706,30 @@
      * @return the instruction on the top of the stack
      */
     public ValueNode pop(Kind slotKind) {
-        assert slotKind.getSlotCount() > 0;
         if (slotKind.needsTwoSlots()) {
             ValueNode s = xpop();
-            assert s == null;
+            assert s == TWO_SLOT_MARKER;
         }
         ValueNode x = xpop();
-        assert x != null;
+        assert verifyKind(slotKind, x);
         return x;
     }
 
     private void xpush(ValueNode x) {
+        assert x != null;
         stack[stackSize++] = x;
     }
 
     private ValueNode xpop() {
-        return stack[--stackSize];
+        ValueNode result = stack[--stackSize];
+        assert result != null;
+        return result;
     }
 
     private ValueNode xpeek() {
-        return stack[stackSize - 1];
+        ValueNode result = stack[stackSize - 1];
+        assert result != null;
+        return result;
     }
 
     /**
@@ -695,16 +740,15 @@
      */
     public ValueNode[] popArguments(int argSize) {
         ValueNode[] result = allocateArray(argSize);
-        int newStackSize = stackSize;
         for (int i = argSize - 1; i >= 0; i--) {
-            newStackSize--;
-            if (stack[newStackSize] == null) {
-                /* Two-slot value. */
-                newStackSize--;
+            ValueNode x = xpop();
+            if (x == TWO_SLOT_MARKER) {
+                /* Ignore second slot of two-slot value. */
+                x = xpop();
             }
-            result[i] = stack[newStackSize];
+            assert x != null && x != TWO_SLOT_MARKER;
+            result[i] = x;
         }
-        stackSize = newStackSize;
         return result;
     }
 
@@ -723,21 +767,26 @@
     public void stackOp(int opcode) {
         switch (opcode) {
             case POP: {
-                xpop();
+                ValueNode w1 = xpop();
+                assert w1 != TWO_SLOT_MARKER;
                 break;
             }
             case POP2: {
                 xpop();
-                xpop();
+                ValueNode w2 = xpop();
+                assert w2 != TWO_SLOT_MARKER;
                 break;
             }
             case DUP: {
-                xpush(xpeek());
+                ValueNode w1 = xpeek();
+                assert w1 != TWO_SLOT_MARKER;
+                xpush(w1);
                 break;
             }
             case DUP_X1: {
                 ValueNode w1 = xpop();
                 ValueNode w2 = xpop();
+                assert w1 != TWO_SLOT_MARKER;
                 xpush(w1);
                 xpush(w2);
                 xpush(w1);
@@ -747,6 +796,7 @@
                 ValueNode w1 = xpop();
                 ValueNode w2 = xpop();
                 ValueNode w3 = xpop();
+                assert w1 != TWO_SLOT_MARKER;
                 xpush(w1);
                 xpush(w3);
                 xpush(w2);
@@ -789,6 +839,8 @@
             case SWAP: {
                 ValueNode w1 = xpop();
                 ValueNode w2 = xpop();
+                assert w1 != TWO_SLOT_MARKER;
+                assert w2 != TWO_SLOT_MARKER;
                 xpush(w1);
                 xpush(w2);
                 break;
@@ -876,11 +928,11 @@
         Debug.log(String.format("|   state [nr locals = %d, stack depth = %d, method = %s]", localsSize(), stackSize(), method));
         for (int i = 0; i < localsSize(); ++i) {
             ValueNode value = locals[i];
-            Debug.log(String.format("|   local[%d] = %-8s : %s", i, value == null ? "bogus" : value.getKind().getJavaName(), value));
+            Debug.log(String.format("|   local[%d] = %-8s : %s", i, value == null ? "bogus" : value == TWO_SLOT_MARKER ? "second" : value.getKind().getJavaName(), value));
         }
         for (int i = 0; i < stackSize(); ++i) {
             ValueNode value = stack[i];
-            Debug.log(String.format("|   stack[%d] = %-8s : %s", i, value == null ? "bogus" : value.getKind().getJavaName(), value));
+            Debug.log(String.format("|   stack[%d] = %-8s : %s", i, value == null ? "bogus" : value == TWO_SLOT_MARKER ? "second" : value.getKind().getJavaName(), value));
         }
     }
 }
--- a/graal/com.oracle.graal.nodes/src/com/oracle/graal/nodes/FrameState.java	Wed Jun 03 02:40:53 2015 +0200
+++ b/graal/com.oracle.graal.nodes/src/com/oracle/graal/nodes/FrameState.java	Tue Jun 02 18:25:16 2015 -0700
@@ -27,6 +27,7 @@
 import java.util.*;
 
 import com.oracle.graal.bytecode.*;
+import com.oracle.graal.compiler.common.type.*;
 import com.oracle.graal.graph.*;
 import com.oracle.graal.graph.iterators.*;
 import com.oracle.graal.nodeinfo.*;
@@ -48,6 +49,22 @@
 
     private static final DebugMetric METRIC_FRAMESTATE_COUNT = Debug.metric("FrameStateCount");
 
+    /**
+     * Marker value for the second slot of values that occupy two local variable or expression stack
+     * slots. The marker value is used by the bytecode parser, but replaced with {@code null} in the
+     * {@link #values} of the {@link FrameState}.
+     */
+    public static final ValueNode TWO_SLOT_MARKER = new TwoSlotMarker();
+
+    @NodeInfo
+    private static final class TwoSlotMarker extends ValueNode {
+        public static final NodeClass<TwoSlotMarker> TYPE = NodeClass.create(TwoSlotMarker.class);
+
+        protected TwoSlotMarker() {
+            super(TYPE, StampFactory.forKind(Kind.Illegal));
+        }
+    }
+
     protected final int localsSize;
 
     protected final int stackSize;
@@ -139,13 +156,23 @@
     private void createValues(ValueNode[] locals, ValueNode[] stack, ValueNode[] locks) {
         int index = 0;
         for (int i = 0; i < locals.length; ++i) {
-            this.values.initialize(index++, locals[i]);
+            ValueNode value = locals[i];
+            if (value == TWO_SLOT_MARKER) {
+                value = null;
+            }
+            this.values.initialize(index++, value);
         }
         for (int i = 0; i < stackSize; ++i) {
-            this.values.initialize(index++, stack[i]);
+            ValueNode value = stack[i];
+            if (value == TWO_SLOT_MARKER) {
+                value = null;
+            }
+            this.values.initialize(index++, value);
         }
         for (int i = 0; i < locks.length; ++i) {
-            this.values.initialize(index++, locks[i]);
+            ValueNode value = locks[i];
+            assert value != TWO_SLOT_MARKER;
+            this.values.initialize(index++, value);
         }
     }
 
--- a/graal/com.oracle.graal.replacements/src/com/oracle/graal/replacements/WordOperationPlugin.java	Wed Jun 03 02:40:53 2015 +0200
+++ b/graal/com.oracle.graal.replacements/src/com/oracle/graal/replacements/WordOperationPlugin.java	Tue Jun 02 18:25:16 2015 -0700
@@ -59,6 +59,11 @@
         this.wordKind = wordTypes.getWordKind();
     }
 
+    @Override
+    public boolean canChangeStackKind(GraphBuilderContext b) {
+        return true;
+    }
+
     /**
      * Processes a call to a method if it is annotated with {@link Operation} by adding nodes to the
      * graph being built that implement the denoted operation.