# HG changeset patch # User Bernhard Urban # Date 1389381391 -3600 # Node ID bd21ee1a874ca8fae6db31b84733a48abba0228b # Parent aaaeef10362551c7e6c315dbcf9b406535635347 fix recursion problem around macro nodes (e.g. array copy) diff -r aaaeef103625 -r bd21ee1a874c graal/com.oracle.graal.hotspot/src/com/oracle/graal/hotspot/replacements/ArrayCopyNode.java --- a/graal/com.oracle.graal.hotspot/src/com/oracle/graal/hotspot/replacements/ArrayCopyNode.java Fri Jan 10 15:26:01 2014 +0100 +++ b/graal/com.oracle.graal.hotspot/src/com/oracle/graal/hotspot/replacements/ArrayCopyNode.java Fri Jan 10 20:16:31 2014 +0100 @@ -107,7 +107,7 @@ final ResolvedJavaMethod snippetMethod = tool.getMetaAccess().lookupJavaMethod(ArrayCopySnippets.genericArraycopySnippet); snippetGraph = null; try (Scope s = Debug.scope("ArrayCopySnippet", snippetMethod)) { - snippetGraph = replacements.getSnippet(snippetMethod).copy(); + snippetGraph = replacements.getSnippet(snippetMethod, getTargetMethod()).copy(); } catch (Throwable e) { throw Debug.handle(e); } diff -r aaaeef103625 -r bd21ee1a874c graal/com.oracle.graal.hotspot/src/com/oracle/graal/hotspot/stubs/GraphKit.java --- a/graal/com.oracle.graal.hotspot/src/com/oracle/graal/hotspot/stubs/GraphKit.java Fri Jan 10 15:26:01 2014 +0100 +++ b/graal/com.oracle.graal.hotspot/src/com/oracle/graal/hotspot/stubs/GraphKit.java Fri Jan 10 20:16:31 2014 +0100 @@ -158,7 +158,7 @@ public void inline(InvokeNode invoke) { ResolvedJavaMethod method = ((MethodCallTargetNode) invoke.callTarget()).targetMethod(); ReplacementsImpl repl = new ReplacementsImpl(providers, new Assumptions(false), providers.getCodeCache().getTarget()); - StructuredGraph calleeGraph = repl.makeGraph(method, null, null, false); + StructuredGraph calleeGraph = repl.makeGraph(method, null, method, null, false, false); InliningUtil.inline(invoke, calleeGraph, false); } } diff -r aaaeef103625 -r bd21ee1a874c graal/com.oracle.graal.nodes/src/com/oracle/graal/nodes/spi/Replacements.java --- a/graal/com.oracle.graal.nodes/src/com/oracle/graal/nodes/spi/Replacements.java Fri Jan 10 15:26:01 2014 +0100 +++ b/graal/com.oracle.graal.nodes/src/com/oracle/graal/nodes/spi/Replacements.java Fri Jan 10 20:16:31 2014 +0100 @@ -42,6 +42,16 @@ StructuredGraph getSnippet(ResolvedJavaMethod method); /** + * Gets the snippet graph derived from a given method. + * + * @param recursiveEntry if the snippet contains a call to this method, it's considered as + * recursive call and won't be processed for {@linkplain MethodSubstitution + * substitutions} or {@linkplain MacroSubstitution macro nodes}. + * @return the snippet graph, if any, that is derived from {@code method} + */ + StructuredGraph getSnippet(ResolvedJavaMethod method, ResolvedJavaMethod recursiveEntry); + + /** * Registers a method as snippet. */ void registerSnippet(ResolvedJavaMethod method); diff -r aaaeef103625 -r bd21ee1a874c graal/com.oracle.graal.replacements.test/src/com/oracle/graal/replacements/test/ObjectAccessTest.java --- a/graal/com.oracle.graal.replacements.test/src/com/oracle/graal/replacements/test/ObjectAccessTest.java Fri Jan 10 15:26:01 2014 +0100 +++ b/graal/com.oracle.graal.replacements.test/src/com/oracle/graal/replacements/test/ObjectAccessTest.java Fri Jan 10 20:16:31 2014 +0100 @@ -54,7 +54,7 @@ @Override protected StructuredGraph parse(Method m) { ResolvedJavaMethod resolvedMethod = getMetaAccess().lookupJavaMethod(m); - return installer.makeGraph(resolvedMethod, null, inliningPolicy.get(), false); + return installer.makeGraph(resolvedMethod, null, resolvedMethod, inliningPolicy.get(), false, false); } @Test diff -r aaaeef103625 -r bd21ee1a874c graal/com.oracle.graal.replacements.test/src/com/oracle/graal/replacements/test/PointerTest.java --- a/graal/com.oracle.graal.replacements.test/src/com/oracle/graal/replacements/test/PointerTest.java Fri Jan 10 15:26:01 2014 +0100 +++ b/graal/com.oracle.graal.replacements.test/src/com/oracle/graal/replacements/test/PointerTest.java Fri Jan 10 20:16:31 2014 +0100 @@ -60,7 +60,7 @@ @Override protected StructuredGraph parse(Method m) { ResolvedJavaMethod resolvedMethod = getMetaAccess().lookupJavaMethod(m); - return installer.makeGraph(resolvedMethod, null, inliningPolicy.get(), false); + return installer.makeGraph(resolvedMethod, null, resolvedMethod, inliningPolicy.get(), false, false); } @Test diff -r aaaeef103625 -r bd21ee1a874c graal/com.oracle.graal.replacements.test/src/com/oracle/graal/replacements/test/WordTest.java --- a/graal/com.oracle.graal.replacements.test/src/com/oracle/graal/replacements/test/WordTest.java Fri Jan 10 15:26:01 2014 +0100 +++ b/graal/com.oracle.graal.replacements.test/src/com/oracle/graal/replacements/test/WordTest.java Fri Jan 10 20:16:31 2014 +0100 @@ -49,7 +49,7 @@ @Override protected StructuredGraph parse(Method m) { ResolvedJavaMethod resolvedMethod = getMetaAccess().lookupJavaMethod(m); - return installer.makeGraph(resolvedMethod, null, inliningPolicy.get(), false); + return installer.makeGraph(resolvedMethod, null, resolvedMethod, inliningPolicy.get(), false, false); } @LongTest diff -r aaaeef103625 -r bd21ee1a874c graal/com.oracle.graal.replacements/src/com/oracle/graal/replacements/ReplacementsImpl.java --- a/graal/com.oracle.graal.replacements/src/com/oracle/graal/replacements/ReplacementsImpl.java Fri Jan 10 15:26:01 2014 +0100 +++ b/graal/com.oracle.graal.replacements/src/com/oracle/graal/replacements/ReplacementsImpl.java Fri Jan 10 20:16:31 2014 +0100 @@ -84,15 +84,19 @@ private static final boolean UseSnippetGraphCache = Boolean.parseBoolean(System.getProperty("graal.useSnippetGraphCache", "true")); private static final DebugTimer SnippetPreparationTime = Debug.timer("SnippetPreparationTime"); + public StructuredGraph getSnippet(ResolvedJavaMethod method) { + return getSnippet(method, null); + } + @Override - public StructuredGraph getSnippet(ResolvedJavaMethod method) { + public StructuredGraph getSnippet(ResolvedJavaMethod method, ResolvedJavaMethod recursiveEntry) { assert method.getAnnotation(Snippet.class) != null : "Snippet must be annotated with @" + Snippet.class.getSimpleName(); assert !Modifier.isAbstract(method.getModifiers()) && !Modifier.isNative(method.getModifiers()) : "Snippet must not be abstract or native"; StructuredGraph graph = UseSnippetGraphCache ? graphs.get(method) : null; if (graph == null) { try (TimerCloseable a = SnippetPreparationTime.start()) { - StructuredGraph newGraph = makeGraph(method, null, inliningPolicy(method), method.getAnnotation(Snippet.class).removeAllFrameStates()); + StructuredGraph newGraph = makeGraph(method, recursiveEntry, recursiveEntry, inliningPolicy(method), method.getAnnotation(Snippet.class).removeAllFrameStates(), false); Debug.metric("SnippetNodeCount[" + method.getName() + "]").add(newGraph.getNodeCount()); if (!UseSnippetGraphCache) { return newGraph; @@ -127,7 +131,7 @@ } StructuredGraph graph = graphs.get(substitute); if (graph == null) { - graphs.putIfAbsent(substitute, makeGraph(substitute, original, inliningPolicy(substitute), false)); + graphs.putIfAbsent(substitute, makeGraph(substitute, original, substitute, inliningPolicy(substitute), false, true)); graph = graphs.get(substitute); } return graph; @@ -259,15 +263,16 @@ * @param policy the inlining policy to use during preprocessing * @param removeAllFrameStates removes all frame states from side effecting instructions */ - public StructuredGraph makeGraph(ResolvedJavaMethod method, ResolvedJavaMethod original, SnippetInliningPolicy policy, boolean removeAllFrameStates) { - return createGraphMaker(method, original).makeGraph(policy, removeAllFrameStates); + public StructuredGraph makeGraph(ResolvedJavaMethod method, ResolvedJavaMethod original, ResolvedJavaMethod recursiveEntry, SnippetInliningPolicy policy, boolean removeAllFrameStates, + boolean isMethodSubstitution) { + return createGraphMaker(method, original, recursiveEntry, isMethodSubstitution).makeGraph(policy, removeAllFrameStates); } /** * Can be overridden to return an object that specializes various parts of graph preprocessing. */ - protected GraphMaker createGraphMaker(ResolvedJavaMethod substitute, ResolvedJavaMethod original) { - return new GraphMaker(substitute, original); + protected GraphMaker createGraphMaker(ResolvedJavaMethod substitute, ResolvedJavaMethod original, ResolvedJavaMethod recursiveEntry, boolean isMethodSubstitution) { + return new GraphMaker(substitute, original, recursiveEntry, isMethodSubstitution); } /** @@ -279,21 +284,32 @@ * Creates and preprocesses a graph for a replacement. */ protected class GraphMaker { - /** * The method for which a graph is being created. */ protected final ResolvedJavaMethod method; /** - * The original method if {@link #method} is a {@linkplain MethodSubstitution substitution} - * otherwise null. + * The method which is used when a call to {@link #recursiveEntry} is found. + */ + protected final ResolvedJavaMethod substitutedMethod; + + /** + * The method which is used to detect a recursive call. */ - protected final ResolvedJavaMethod original; + protected final ResolvedJavaMethod recursiveEntry; - protected GraphMaker(ResolvedJavaMethod substitute, ResolvedJavaMethod original) { + /** + * Controls if FrameStates should be removed or processed with the + * {@link SnippetFrameStateCleanupPhase}. + */ + protected final boolean isMethodSubstitution; + + protected GraphMaker(ResolvedJavaMethod substitute, ResolvedJavaMethod substitutedMethod, ResolvedJavaMethod recursiveEntry, boolean isMethodSubstitution) { this.method = substitute; - this.original = original; + this.substitutedMethod = substitutedMethod; + this.recursiveEntry = recursiveEntry; + this.isMethodSubstitution = isMethodSubstitution; } public StructuredGraph makeGraph(final SnippetInliningPolicy policy, final boolean removeAllFrameStates) { @@ -323,7 +339,7 @@ } new ConvertDeoptimizeToGuardPhase().apply(graph); - if (original == null) { + if (!isMethodSubstitution) { if (removeAllFrameStates) { for (Node node : graph.getNodes()) { if (node instanceof StateSplit) { @@ -403,18 +419,20 @@ } private StructuredGraph buildGraph(final ResolvedJavaMethod methodToParse, final SnippetInliningPolicy policy) { - assert !Modifier.isAbstract(methodToParse.getModifiers()) && !Modifier.isNative(methodToParse.getModifiers()) : methodToParse; + assert isInlinableSnippet(methodToParse) : methodToParse; final StructuredGraph graph = buildInitialGraph(methodToParse); try (Scope s = Debug.scope("buildGraph", graph)) { for (MethodCallTargetNode callTarget : graph.getNodes(MethodCallTargetNode.class)) { ResolvedJavaMethod callee = callTarget.targetMethod(); - if (callee == method) { - final StructuredGraph originalGraph = buildInitialGraph(original); - InliningUtil.inline(callTarget.invoke(), originalGraph, true); + if (callee == recursiveEntry) { + if (isInlinableSnippet(substitutedMethod)) { + final StructuredGraph originalGraph = buildInitialGraph(substitutedMethod); + InliningUtil.inline(callTarget.invoke(), originalGraph, true); - Debug.dump(graph, "after inlining %s", callee); - afterInline(graph, originalGraph, null); + Debug.dump(graph, "after inlining %s", callee); + afterInline(graph, originalGraph, null); + } } else { Class macroNodeClass = InliningUtil.getMacroNodeClass(ReplacementsImpl.this, callee); if (macroNodeClass != null) { @@ -457,6 +475,10 @@ } } + private static boolean isInlinableSnippet(final ResolvedJavaMethod methodToParse) { + return !Modifier.isAbstract(methodToParse.getModifiers()) && !Modifier.isNative(methodToParse.getModifiers()); + } + private static String originalName(Method substituteMethod, String methodSubstitution) { if (methodSubstitution.isEmpty()) { return substituteMethod.getName();