Skip to content

Commit 44ce3fa

Browse files
committed
[GR-45416] CompilationAlarm shouldn't interfere with snippet creation
PullRequest: graal/14322
2 parents f03de5b + 24ac40a commit 44ce3fa

File tree

5 files changed

+76
-45
lines changed

5 files changed

+76
-45
lines changed

compiler/src/jdk.graal.compiler.test/src/jdk/graal/compiler/debug/test/CompilationAlarmTest.java

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,8 @@
2626

2727
import java.util.Optional;
2828

29+
import org.junit.Test;
30+
2931
import jdk.graal.compiler.api.directives.GraalDirectives;
3032
import jdk.graal.compiler.code.CompilationResult;
3133
import jdk.graal.compiler.core.GraalCompiler;
@@ -41,8 +43,6 @@
4143
import jdk.graal.compiler.phases.BasePhase;
4244
import jdk.graal.compiler.phases.tiers.LowTierContext;
4345
import jdk.graal.compiler.phases.tiers.Suites;
44-
import org.junit.Test;
45-
4646
import jdk.vm.ci.meta.ResolvedJavaMethod;
4747

4848
public class CompilationAlarmTest extends GraalCompilerTest {
@@ -68,7 +68,7 @@ protected void run(StructuredGraph graph, LowTierContext context) {
6868
long start = System.currentTimeMillis();
6969
try {
7070
while (System.currentTimeMillis() - start < end) {
71-
CompilationAlarm.current().checkExpiration();
71+
CompilationAlarm.checkProgress(graph);
7272
if (withProgressCounterEvents) {
7373
CompilationAlarm.checkProgress(graph);
7474
testThread.events++;

compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/core/common/util/CompilationAlarm.java

Lines changed: 24 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -66,6 +66,7 @@ public static class Options {
6666
private CompilationAlarm(double period) {
6767
this.period = period;
6868
this.expiration = period == 0.0D ? 0L : System.currentTimeMillis() + (long) (period * 1000);
69+
this.previous = currentAlarm.get();
6970
}
7071

7172
/**
@@ -97,25 +98,22 @@ public double getPeriod() {
9798
* @return {@code true} if this alarm has been active for more than {@linkplain #getPeriod()
9899
* period} seconds, {@code false} otherwise
99100
*/
100-
public boolean hasExpired() {
101-
return this != NEVER_EXPIRES && System.currentTimeMillis() > expiration;
101+
private boolean hasExpired() {
102+
return expiration != 0 && System.currentTimeMillis() > expiration;
102103
}
103104

104105
/**
105106
* Checks whether this alarm {@link #hasExpired()} and if so, raises a bailout exception.
106107
*/
107-
public void checkExpiration() {
108+
private void checkExpiration() {
108109
if (hasExpired()) {
109110
throw new PermanentBailoutException("Compilation exceeded %.3f seconds", period);
110111
}
111112
}
112113

113114
@Override
114115
public void close() {
115-
if (this != NEVER_EXPIRES) {
116-
currentAlarm.set(null);
117-
resetProgressDetection();
118-
}
116+
currentAlarm.set(previous);
119117
}
120118

121119
/**
@@ -128,10 +126,15 @@ public void close() {
128126
*/
129127
private final long expiration;
130128

129+
/**
130+
* The previously installed alarm.
131+
*/
132+
private final CompilationAlarm previous;
133+
131134
/**
132135
* Starts an alarm for setting a time limit on a compilation if there isn't already an active
133136
* alarm and {@link CompilationAlarm.Options#CompilationExpirationPeriod}{@code > 0}. The
134-
* returned value can be used in a try-with-resource statement to disable the alarm once the
137+
* returned value should be used in a try-with-resource statement to disable the alarm once the
135138
* compilation is finished.
136139
*
137140
* @return a {@link CompilationAlarm} if there was no current alarm for the calling thread
@@ -146,16 +149,23 @@ public static CompilationAlarm trackCompilationPeriod(OptionValues options) {
146149
if (Assertions.detailedAssertionsEnabled(options)) {
147150
period *= 2;
148151
}
149-
CompilationAlarm current = currentAlarm.get();
150-
if (current == null) {
151-
current = new CompilationAlarm(period);
152-
currentAlarm.set(current);
153-
return current;
154-
}
152+
CompilationAlarm current = new CompilationAlarm(period);
153+
currentAlarm.set(current);
154+
return current;
155155
}
156156
return null;
157157
}
158158

159+
/**
160+
* Disable the compilation alarm. The returned value should be used in a try-with-resource
161+
* statement to restore the previous alarm state.
162+
*/
163+
public static CompilationAlarm disable() {
164+
CompilationAlarm current = new CompilationAlarm(0);
165+
currentAlarm.set(current);
166+
return current;
167+
}
168+
159169
/**
160170
* Number of graph events (iterating inputs, usages, etc) before triggering a check on the
161171
* compilation alarm.

compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/phases/graph/ReentrantBlockIterator.java

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -136,12 +136,9 @@ public static <StateT> EconomicMap<FixedNode, StateT> apply(BlockIteratorClosure
136136
StateT state = initialState;
137137
HIRBlock current = start;
138138

139-
CompilationAlarm compilationAlarm = CompilationAlarm.current();
140-
141139
while (true) { // TERMINATION ARGUMENT: processing all blocks reverse post order until end
142140
// of cfg or until a bailout is triggered because of a long compile
143141
CompilationAlarm.checkProgress(start.getCfg().graph);
144-
compilationAlarm.checkExpiration();
145142
HIRBlock next = null;
146143
if (stopAtBlock != null && stopAtBlock.test(current)) {
147144
states.put(current.getBeginNode(), state);

compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/replacements/SnippetTemplate.java

Lines changed: 45 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,7 @@
7070
import jdk.graal.compiler.core.common.type.StampFactory;
7171
import jdk.graal.compiler.core.common.type.StampPair;
7272
import jdk.graal.compiler.core.common.type.TypeReference;
73+
import jdk.graal.compiler.core.common.util.CompilationAlarm;
7374
import jdk.graal.compiler.debug.Assertions;
7475
import jdk.graal.compiler.debug.CounterKey;
7576
import jdk.graal.compiler.debug.DebugCloseable;
@@ -528,6 +529,15 @@ protected SnippetInfo(ResolvedJavaMethod method, ResolvedJavaMethod original, Lo
528529
this.type = type;
529530
}
530531

532+
public boolean isPrivateLocation(LocationIdentity identity) {
533+
for (LocationIdentity i : privateLocations) {
534+
if (i.equals(identity)) {
535+
return true;
536+
}
537+
}
538+
return false;
539+
}
540+
531541
public ResolvedJavaMethod getMethod() {
532542
return method;
533543
}
@@ -983,7 +993,8 @@ public SnippetTemplate template(CoreProviders context, ValueNode replacee, final
983993
try (DebugContext debug = context.getReplacements().openSnippetDebugContext("SnippetTemplate_", args.cacheKey.method, outer, options)) {
984994
try (DebugCloseable a = SnippetTemplateCreationTime.start(outer);
985995
DebugCloseable a2 = args.info.creationTimer.start(outer);
986-
DebugContext.Scope s = debug.scope("SnippetSpecialization", args.info.method)) {
996+
DebugContext.Scope s = debug.scope("SnippetSpecialization", args.info.method);
997+
CompilationAlarm alarm = CompilationAlarm.disable()) {
987998
SnippetTemplates.increment(outer);
988999
args.info.creationCounter.increment(outer);
9891000
OptionValues snippetOptions = new OptionValues(options, GraalOptions.TraceInlining, GraalOptions.TraceInliningForStubsAndSnippets.getValue(options),
@@ -1062,7 +1073,7 @@ protected SnippetTemplate(OptionValues options,
10621073
SnippetReflectionProvider snippetReflection,
10631074
Arguments args,
10641075
boolean trackNodeSourcePosition,
1065-
Node replacee,
1076+
ValueNode replacee,
10661077
PhaseSuite<CoreProviders> midTierPreLoweringPhases,
10671078
PhaseSuite<CoreProviders> midTierPostLoweringPhases) {
10681079
this.snippetReflection = snippetReflection;
@@ -1236,10 +1247,9 @@ protected SnippetTemplate(OptionValues options,
12361247
boolean needsCE = false;
12371248
LoweringTool.LoweringStage loweringStage = args.cacheKey.loweringStage;
12381249
for (Node n : snippetCopy.getNodes()) {
1239-
if (n instanceof AbstractNewObjectNode || n instanceof AbstractBoxingNode) {
1250+
if (!needsPEA && (n instanceof AbstractNewObjectNode || n instanceof AbstractBoxingNode)) {
12401251
needsPEA = true;
1241-
break;
1242-
} else if (n instanceof LogicNode) {
1252+
} else if (!needsCE && n instanceof LogicNode) {
12431253
needsCE = true;
12441254
}
12451255
}
@@ -1328,11 +1338,18 @@ protected SnippetTemplate(OptionValues options,
13281338

13291339
if (node instanceof StateSplit) {
13301340
StateSplit stateSplit = (StateSplit) node;
1331-
FrameState frameState = stateSplit.stateAfter();
1332-
if (stateSplit.hasSideEffect()) {
1341+
1342+
// The normal side effect check doesn't understand private locations, so
1343+
// explicitly filter those accesses from the side effects.
1344+
boolean hasSideEffect = stateSplit.hasSideEffect();
1345+
if (hasSideEffect && stateSplit instanceof MemoryAccess && MemoryKill.isSingleMemoryKill(stateSplit.asNode())) {
1346+
hasSideEffect = !info.isPrivateLocation(((SingleMemoryKill) stateSplit).getKilledLocationIdentity());
1347+
}
1348+
if (hasSideEffect) {
13331349
curSideEffectNodes.add((StateSplit) node);
13341350
}
13351351
if (info.type == SnippetType.INLINED_SNIPPET) {
1352+
FrameState frameState = stateSplit.stateAfter();
13361353
if (frameState != null) {
13371354
stateSplit.setStateAfter(null);
13381355
}
@@ -1495,6 +1512,10 @@ protected SnippetTemplate(OptionValues options,
14951512
}
14961513
}
14971514

1515+
if (!curSideEffectNodes.isEmpty()) {
1516+
checkSideEffects(replacee);
1517+
}
1518+
14981519
debug.dump(DebugContext.INFO_LEVEL, snippet, "SnippetTemplate final state");
14991520
assert snippet.verify();
15001521
this.snippet.freeze();
@@ -2129,21 +2150,7 @@ public UnmodifiableEconomicMap<Node, Node> instantiate(MetaAccessProvider metaAc
21292150
FixedNode firstCFGNodeDuplicate = (FixedNode) duplicates.get(firstCFGNode);
21302151
replacee.replaceAtPredecessor(firstCFGNodeDuplicate);
21312152

2132-
if (replacee.graph().getGuardsStage().areFrameStatesAtSideEffects()) {
2133-
boolean replaceeHasSideEffect = replacee instanceof StateSplit && ((StateSplit) replacee).hasSideEffect();
2134-
boolean replacementHasSideEffect = !sideEffectNodes.isEmpty();
2135-
2136-
/*
2137-
* Following cases are allowed: Either the replacee and replacement don't have
2138-
* side-effects or the replacee has and the replacement hasn't (lowered to something
2139-
* without a side-effect which is fine regarding correctness) or both have
2140-
* side-effects, under which conditions also merges should have states.
2141-
*/
2142-
2143-
if (replacementHasSideEffect) {
2144-
GraalError.guarantee(replaceeHasSideEffect, "Lowering node %s without side-effect to snippet %s with sideeffects=%s", replacee, info, this.sideEffectNodes);
2145-
}
2146-
}
2153+
checkSideEffects(replacee);
21472154

21482155
updateStamps(replacee, duplicates);
21492156

@@ -2316,6 +2323,22 @@ public static FixedNode walkBackToExceptionEdgeStart(FixedNode start) {
23162323
return null;
23172324
}
23182325

2326+
/**
2327+
* Check that either the replacee and replacement don't have side effects or the replacee does
2328+
* and the replacement doesn't (lowered to something without a side effect which is fine
2329+
* regarding correctness) or both have side effects, under which conditions also merges should
2330+
* have states.
2331+
*/
2332+
private void checkSideEffects(ValueNode replacee) {
2333+
if (replacee.graph().getGuardsStage().areFrameStatesAtSideEffects()) {
2334+
boolean replaceeHasSideEffect = replacee instanceof StateSplit && ((StateSplit) replacee).hasSideEffect();
2335+
boolean replacementHasSideEffect = !sideEffectNodes.isEmpty();
2336+
if (replacementHasSideEffect) {
2337+
GraalError.guarantee(replaceeHasSideEffect, "Lowering node %s without side effect to snippet %s with side effects=%s", replacee, info, this.sideEffectNodes);
2338+
}
2339+
}
2340+
}
2341+
23192342
/**
23202343
* Searches for {@link WithExceptionNode} reachable from the {@link UnwindNode} and marks them
23212344
* as {@linkplain WithExceptionNode#replaceWithNonThrowing() non-throwing}.

compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/virtual/phases/ea/EffectsPhase.java

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -29,10 +29,11 @@
2929
import java.util.Optional;
3030

3131
import org.graalvm.collections.EconomicSet;
32+
3233
import jdk.graal.compiler.core.common.util.CompilationAlarm;
3334
import jdk.graal.compiler.debug.DebugContext;
34-
import jdk.graal.compiler.graph.Graph.NodeEventScope;
3535
import jdk.graal.compiler.graph.Graph;
36+
import jdk.graal.compiler.graph.Graph.NodeEventScope;
3637
import jdk.graal.compiler.graph.Node;
3738
import jdk.graal.compiler.nodes.GraphState;
3839
import jdk.graal.compiler.nodes.StructuredGraph;
@@ -95,9 +96,9 @@ public boolean runAnalysis(StructuredGraph graph, CoreProvidersT context) {
9596
LoopUtility.removeObsoleteProxies(graph, context, canonicalizer);
9697
assert unscheduled || strategy != null;
9798
boolean changed = false;
98-
CompilationAlarm compilationAlarm = CompilationAlarm.current();
9999
DebugContext debug = graph.getDebug();
100-
for (int iteration = 0; iteration < maxIterations && !compilationAlarm.hasExpired(); iteration++) {
100+
for (int iteration = 0; iteration < maxIterations; iteration++) {
101+
CompilationAlarm.checkProgress(graph);
101102
try (DebugContext.Scope s = debug.scope(debug.areScopesEnabled() ? "iteration " + iteration : null)) {
102103
ScheduleResult schedule;
103104
ControlFlowGraph cfg;

0 commit comments

Comments
 (0)