Skip to content

Commit

Permalink
prevent macronodes in deoptimization targets
Browse files Browse the repository at this point in the history
  • Loading branch information
@teshull
teshull committed Feb 1, 2024
1 parent abc1684 commit c8dd90a
Show file tree
Hide file tree
Showing 10 changed files with 125 additions and 74 deletions.
2 changes: 1 addition & 1 deletion ...iler/src/jdk/graal/compiler/hotspot/replacements/HotSpotReflectionGetCallerClassNode.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,8 @@

import jdk.graal.compiler.graph.NodeClass;
import jdk.graal.compiler.nodeinfo.NodeInfo;
import jdk.graal.compiler.replacements.nodes.MacroNode.MacroParams;
import jdk.graal.compiler.replacements.nodes.ReflectionGetCallerClassNode;

import jdk.vm.ci.hotspot.HotSpotResolvedJavaMethod;
import jdk.vm.ci.meta.MetaAccessProvider;
import jdk.vm.ci.meta.ResolvedJavaMethod;
Expand Down
26 changes: 20 additions & 6 deletions ...c/jdk.graal.compiler/src/jdk/graal/compiler/replacements/nodes/ArrayIndexOfMacroNode.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,8 @@
import jdk.graal.compiler.nodes.ConstantNode;
import jdk.graal.compiler.nodes.ValueNode;
import jdk.graal.compiler.nodes.spi.LoweringTool;
import jdk.graal.compiler.nodes.spi.Simplifiable;
import jdk.graal.compiler.nodes.spi.SimplifierTool;
import jdk.vm.ci.code.Architecture;
import jdk.vm.ci.meta.JavaKind;

Expand All @@ -47,31 +49,42 @@
* image baseline CPU feature set does not meet its requirements.
*/
@NodeInfo(cycles = NodeCycles.CYCLES_UNKNOWN, size = NodeSize.SIZE_128)
public final class ArrayIndexOfMacroNode extends MacroNode {
public final class ArrayIndexOfMacroNode extends MacroWithExceptionNode implements Simplifiable {

public static final NodeClass<ArrayIndexOfMacroNode> TYPE = NodeClass.create(ArrayIndexOfMacroNode.class);

private final Stride stride;
private final LIRGeneratorTool.ArrayIndexOfVariant variant;

private final LocationIdentity locationIdentity;
private boolean intrinsifiable = false;

public ArrayIndexOfMacroNode(MacroParams p, Stride stride, LIRGeneratorTool.ArrayIndexOfVariant variant, LocationIdentity locationIdentity) {
public ArrayIndexOfMacroNode(MacroNode.MacroParams p, Stride stride, LIRGeneratorTool.ArrayIndexOfVariant variant, LocationIdentity locationIdentity) {
this(TYPE, p, stride, variant, locationIdentity);
}

public ArrayIndexOfMacroNode(NodeClass<? extends MacroNode> c, MacroParams p, Stride stride, LIRGeneratorTool.ArrayIndexOfVariant variant,
public ArrayIndexOfMacroNode(NodeClass<? extends MacroWithExceptionNode> c, MacroNode.MacroParams p, Stride stride, LIRGeneratorTool.ArrayIndexOfVariant variant,
LocationIdentity locationIdentity) {
super(c, p);
this.stride = stride;
this.variant = variant;
this.locationIdentity = locationIdentity;
}

@Override
public void simplify(SimplifierTool tool) {
if (!intrinsifiable) {
Architecture arch = tool.getLowerer().getTarget().arch;
if (ArrayIndexOfNode.isSupported(arch, stride, variant)) {
intrinsifiable = true;
replaceWithNonThrowing();
}
}
}

@Override
public void lower(LoweringTool tool) {
Architecture arch = tool.getLowerer().getTarget().arch;
if (ArrayIndexOfNode.isSupported(arch, stride, variant)) {
if (intrinsifiable) {
// some arguments of the original method are unused in the intrinsic. original args:
// 0: Node location
// 1: array
Expand All @@ -95,7 +108,8 @@ public void lower(LoweringTool tool) {
getArgument(6), // fromIndex
searchValues // values
));
graph().replaceFixedWithFixed(this, replacement);
killExceptionEdge();
graph().replaceSplitWithFixed(this, replacement, next());
if (variant == LIRGeneratorTool.ArrayIndexOfVariant.Table) {
graph().addBeforeFixed(replacement, (ComputeObjectAddressNode) searchValues[0]);
}
Expand Down
34 changes: 21 additions & 13 deletions ...aal.compiler/src/jdk/graal/compiler/replacements/nodes/CalcStringAttributesMacroNode.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,14 +24,16 @@
*/
package jdk.graal.compiler.replacements.nodes;

import org.graalvm.word.LocationIdentity;

import jdk.graal.compiler.graph.Node;
import jdk.graal.compiler.graph.NodeClass;
import jdk.graal.compiler.lir.gen.LIRGeneratorTool;
import jdk.graal.compiler.nodeinfo.NodeCycles;
import jdk.graal.compiler.nodeinfo.NodeInfo;
import jdk.graal.compiler.nodeinfo.NodeSize;
import jdk.graal.compiler.nodes.spi.LoweringTool;
import org.graalvm.word.LocationIdentity;

import jdk.graal.compiler.nodes.spi.Canonicalizable;
import jdk.graal.compiler.nodes.spi.CanonicalizerTool;
import jdk.vm.ci.aarch64.AArch64;
import jdk.vm.ci.amd64.AMD64;
import jdk.vm.ci.code.Architecture;
Expand All @@ -43,19 +45,19 @@
* {@code AMD64CalcStringAttributesOp} for details.
*/
@NodeInfo(cycles = NodeCycles.CYCLES_UNKNOWN, size = NodeSize.SIZE_128)
public final class CalcStringAttributesMacroNode extends MacroNode {
public final class CalcStringAttributesMacroNode extends MacroWithExceptionNode implements Canonicalizable {

public static final NodeClass<CalcStringAttributesMacroNode> TYPE = NodeClass.create(CalcStringAttributesMacroNode.class);

private final LIRGeneratorTool.CalcStringAttributesEncoding encoding;
private final boolean assumeValid;
private final LocationIdentity locationIdentity;

public CalcStringAttributesMacroNode(MacroParams p, LIRGeneratorTool.CalcStringAttributesEncoding encoding, boolean assumeValid, LocationIdentity locationIdentity) {
public CalcStringAttributesMacroNode(MacroNode.MacroParams p, LIRGeneratorTool.CalcStringAttributesEncoding encoding, boolean assumeValid, LocationIdentity locationIdentity) {
this(TYPE, p, encoding, assumeValid, locationIdentity);
}

public CalcStringAttributesMacroNode(NodeClass<? extends MacroNode> c, MacroParams p, LIRGeneratorTool.CalcStringAttributesEncoding encoding, boolean assumeValid,
public CalcStringAttributesMacroNode(NodeClass<? extends MacroWithExceptionNode> c, MacroNode.MacroParams p, LIRGeneratorTool.CalcStringAttributesEncoding encoding, boolean assumeValid,
LocationIdentity locationIdentity) {
super(c, p);
this.encoding = encoding;
Expand All @@ -64,14 +66,20 @@ public CalcStringAttributesMacroNode(NodeClass<? extends MacroNode> c, MacroPara
}

@Override
public void lower(LoweringTool tool) {
public Node canonical(CanonicalizerTool tool) {
/*
* This node has architecture-specific feature checks. However, since all logic besides the
* feature check is shared, we have deliberately decided avoid code duplication and not to
* make architecture-specific nodes.
*/
Architecture arch = tool.getLowerer().getTarget().arch;
if (arch instanceof AMD64 && ((AMD64) arch).getFeatures().containsAll(CalcStringAttributesNode.minFeaturesAMD64()) ||
arch instanceof AArch64 && ((AArch64) arch).getFeatures().containsAll(CalcStringAttributesNode.minFeaturesAARCH64())) {
CalcStringAttributesNode replacement = graph().addOrUnique(new CalcStringAttributesNode(getArgument(1), getArgument(2), getArgument(3), encoding, assumeValid, locationIdentity));
graph().replaceFixedWithFixed(this, replacement);
} else {
super.lower(tool);
boolean intrinsifiable = (arch instanceof AMD64 && ((AMD64) arch).getFeatures().containsAll(CalcStringAttributesNode.minFeaturesAMD64())) ||
(arch instanceof AArch64 && ((AArch64) arch).getFeatures().containsAll(CalcStringAttributesNode.minFeaturesAARCH64()));
if (intrinsifiable) {
return new CalcStringAttributesNode(getArgument(1), getArgument(2), getArgument(3), encoding, assumeValid, locationIdentity);

}

return this;
}
}
18 changes: 2 additions & 16 deletions ...raal.compiler/src/jdk/graal/compiler/replacements/nodes/ReflectionGetCallerClassNode.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,20 +32,17 @@
import jdk.graal.compiler.nodes.FrameState;
import jdk.graal.compiler.nodes.spi.Canonicalizable;
import jdk.graal.compiler.nodes.spi.CanonicalizerTool;
import jdk.graal.compiler.nodes.spi.Lowerable;
import jdk.graal.compiler.nodes.spi.LoweringTool;

import jdk.vm.ci.meta.ConstantReflectionProvider;
import jdk.vm.ci.meta.MetaAccessProvider;
import jdk.vm.ci.meta.ResolvedJavaMethod;
import jdk.vm.ci.meta.ResolvedJavaType;

@NodeInfo
public abstract class ReflectionGetCallerClassNode extends MacroNode implements Canonicalizable, Lowerable {
public abstract class ReflectionGetCallerClassNode extends MacroWithExceptionNode implements Canonicalizable {

public static final NodeClass<ReflectionGetCallerClassNode> TYPE = NodeClass.create(ReflectionGetCallerClassNode.class);

protected ReflectionGetCallerClassNode(NodeClass<? extends ReflectionGetCallerClassNode> c, MacroParams p) {
protected ReflectionGetCallerClassNode(NodeClass<? extends ReflectionGetCallerClassNode> c, MacroNode.MacroParams p) {
super(c, p);
}

Expand All @@ -58,17 +55,6 @@ public Node canonical(CanonicalizerTool tool) {
return this;
}

@Override
public void lower(LoweringTool tool) {
ConstantNode callerClassNode = getCallerClassNode(tool.getMetaAccess(), tool.getConstantReflection());

if (callerClassNode != null) {
graph().replaceFixedWithFloating(this, graph().addOrUniqueWithInputs(callerClassNode));
} else {
super.lower(tool);
}
}

/**
* If inlining is deep enough this method returns a {@link ConstantNode} of the caller class by
* walking the stack.
Expand Down
6 changes: 3 additions & 3 deletions ...e.svm.core/src/com/oracle/svm/core/graal/nodes/SubstrateReflectionGetCallerClassNode.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,12 +24,12 @@
*/
package com.oracle.svm.core.graal.nodes;

import com.oracle.svm.core.jdk.StackTraceUtils;

import jdk.graal.compiler.graph.NodeClass;
import jdk.graal.compiler.nodeinfo.NodeInfo;
import jdk.graal.compiler.replacements.nodes.MacroNode.MacroParams;
import jdk.graal.compiler.replacements.nodes.ReflectionGetCallerClassNode;

import com.oracle.svm.core.jdk.StackTraceUtils;

import jdk.vm.ci.meta.MetaAccessProvider;
import jdk.vm.ci.meta.ResolvedJavaMethod;

Expand Down
7 changes: 7 additions & 0 deletions ...m.graal/src/com/oracle/svm/graal/hosted/runtimecompilation/RuntimeCompilationFeature.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -752,6 +752,13 @@ public boolean validateGraph(PointsToAnalysis bb, StructuredGraph graph) {
return true;
}

@Override
public void afterParsingHook(AnalysisMethod method, StructuredGraph graph) {
if (method.isDeoptTarget()) {
new RuntimeCompiledMethodSupport.ConvertMacroNodes().apply(graph);
}
}

@Override
public void initializeInlineBeforeAnalysisPolicy(SVMHost svmHost, InlineBeforeAnalysisPolicyUtils inliningUtils) {
if (RuntimeCompilationFeature.Options.RuntimeCompilationInlineBeforeAnalysis.getValue()) {
Expand Down
22 changes: 22 additions & 0 deletions ...raal/src/com/oracle/svm/graal/hosted/runtimecompilation/RuntimeCompiledMethodSupport.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -71,13 +71,15 @@
import jdk.graal.compiler.core.common.spi.ConstantFieldProvider;
import jdk.graal.compiler.debug.DebugContext;
import jdk.graal.compiler.debug.DebugHandlersFactory;
import jdk.graal.compiler.graph.Node;
import jdk.graal.compiler.java.BytecodeParser;
import jdk.graal.compiler.java.GraphBuilderPhase;
import jdk.graal.compiler.loop.phases.ConvertDeoptimizeToGuardPhase;
import jdk.graal.compiler.nodes.CallTargetNode;
import jdk.graal.compiler.nodes.FrameState;
import jdk.graal.compiler.nodes.GraphDecoder;
import jdk.graal.compiler.nodes.GraphEncoder;
import jdk.graal.compiler.nodes.InvokeWithExceptionNode;
import jdk.graal.compiler.nodes.StateSplit;
import jdk.graal.compiler.nodes.StructuredGraph;
import jdk.graal.compiler.nodes.ValueNode;
Expand All @@ -96,6 +98,8 @@
import jdk.graal.compiler.phases.tiers.HighTierContext;
import jdk.graal.compiler.phases.util.Providers;
import jdk.graal.compiler.printer.GraalDebugHandlersFactory;
import jdk.graal.compiler.replacements.nodes.MacroNode;
import jdk.graal.compiler.replacements.nodes.MacroWithExceptionNode;
import jdk.graal.compiler.truffle.nodes.ObjectLocationIdentity;
import jdk.vm.ci.code.Architecture;
import jdk.vm.ci.code.BytecodeFrame;
Expand Down Expand Up @@ -467,6 +471,24 @@ protected boolean shouldVerifyFrameStates() {
}
}

/**
* Converts {@link MacroWithExceptionNode}s into explicit {@link InvokeWithExceptionNode}s. This
* is necessary to ensure a MacroNode within runtime compilation converted back to an invoke
* will always have a proper deoptimization target.
*/
public static class ConvertMacroNodes extends Phase {
@Override
protected void run(StructuredGraph graph) {
for (Node n : graph.getNodes().snapshot()) {
VMError.guarantee(!(n instanceof MacroNode), "DeoptTarget Methods do not support Macro Nodes: method %s, node %s", graph.method(), n);

if (n instanceof MacroWithExceptionNode macro) {
macro.replaceWithInvoke();
}
}
}
}

/**
* Removes {@link DeoptEntryNode}s, {@link DeoptProxyAnchorNode}s, and {@link DeoptProxyNode}s
* which are determined to be unnecessary after the runtime compilation methods are optimized.
Expand Down
4 changes: 4 additions & 0 deletions substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/SVMHost.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -550,6 +550,10 @@ public void methodAfterParsingHook(BigBang bb, AnalysisMethod method, Structured
graph.getGraphState().configureExplicitExceptionsNoDeoptIfNecessary();
}

if (parsingSupport != null) {
parsingSupport.afterParsingHook(method, graph);
}

if (!SubstrateCompilationDirectives.isRuntimeCompiledMethod(method)) {
/*
* Runtime compiled methods should not have assertions. If they do, then they should
Expand Down
2 changes: 2 additions & 0 deletions ...atevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/analysis/SVMParsingSupport.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,8 @@ public interface SVMParsingSupport {

boolean validateGraph(PointsToAnalysis bb, StructuredGraph graph);

void afterParsingHook(AnalysisMethod method, StructuredGraph graph);

boolean allowAssumptions(AnalysisMethod method);

boolean recordInlinedMethods(AnalysisMethod method);
Expand Down
Loading

0 comments on commit c8dd90a

Please sign in to comment.