Skip to content

Commit 7acbd59

Browse files
d-kozakd-kozak
committed
process feedback from reviews
1 parent 9a8f201 commit 7acbd59

File tree

13 files changed

+118
-63
lines changed

13 files changed

+118
-63
lines changed

‎compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/nodes/InvokeNode.java‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -129,10 +129,6 @@ public void setPolymorphic(boolean value) {
129129
this.polymorphic = value;
130130
}
131131

132-
public void setKilledLocationIdentity(LocationIdentity identity) {
133-
this.killedLocationIdentity = identity;
134-
}
135-
136132
@Override
137133
public void setInlineControl(InlineControl control) {
138134
this.inlineControl = control;
@@ -157,6 +153,10 @@ public LocationIdentity getKilledLocationIdentity() {
157153
return killedLocationIdentity;
158154
}
159155

156+
public void setKilledLocationIdentity(LocationIdentity identity) {
157+
this.killedLocationIdentity = identity;
158+
}
159+
160160
@Override
161161
public void generate(NodeLIRBuilderTool gen) {
162162
gen.emitInvoke(this);

‎compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/nodes/InvokeWithExceptionNode.java‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -167,6 +167,10 @@ public boolean hasSideEffect() {
167167
return sideEffect;
168168
}
169169

170+
public void setSideEffect(boolean withSideEffects) {
171+
this.sideEffect = withSideEffects;
172+
}
173+
170174
@Override
171175
public LocationIdentity getKilledLocationIdentity() {
172176
return killedLocationIdentity;
@@ -274,8 +278,4 @@ public boolean isInOOMETry() {
274278
public void setInOOMETry(boolean isInOOMETry) {
275279
this.isInOOMETry = isInOOMETry;
276280
}
277-
278-
public void setSideEffect(boolean withSideEffects) {
279-
this.sideEffect = withSideEffects;
280-
}
281281
}

‎substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/NativeImageGenerator.java‎

Lines changed: 13 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,6 @@
5353
import java.util.function.BooleanSupplier;
5454
import java.util.function.Function;
5555

56-
import com.oracle.svm.hosted.reflect.ReflectionFeature;
5756
import org.graalvm.collections.EconomicSet;
5857
import org.graalvm.collections.Pair;
5958
import org.graalvm.nativeimage.ImageInfo;
@@ -200,7 +199,7 @@
200199
import com.oracle.svm.hosted.analysis.ReachabilityTracePrinter;
201200
import com.oracle.svm.hosted.analysis.SVMAnalysisMetaAccess;
202201
import com.oracle.svm.hosted.analysis.SubstrateUnsupportedFeatures;
203-
import com.oracle.svm.hosted.analysis.tesa.TransitiveEffectSummaryAnalysisEngine;
202+
import com.oracle.svm.hosted.analysis.tesa.TesaEngine;
204203
import com.oracle.svm.hosted.annotation.SubstrateAnnotationExtractor;
205204
import com.oracle.svm.hosted.c.CAnnotationProcessorCache;
206205
import com.oracle.svm.hosted.c.CConstantValueSupportImpl;
@@ -254,6 +253,7 @@
254253
import com.oracle.svm.hosted.phases.VerifyDeoptLIRFrameStatesPhase;
255254
import com.oracle.svm.hosted.phases.VerifyNoGuardsPhase;
256255
import com.oracle.svm.hosted.pltgot.PLTGOTOptions;
256+
import com.oracle.svm.hosted.reflect.ReflectionFeature;
257257
import com.oracle.svm.hosted.reflect.proxy.ProxyRenamingSubstitutionProcessor;
258258
import com.oracle.svm.hosted.snippets.SubstrateGraphBuilderPlugins;
259259
import com.oracle.svm.hosted.substitute.AnnotationSubstitutionProcessor;
@@ -595,13 +595,13 @@ protected void doRun(Map<Method, CEntryPointData> entryPoints, JavaMainSupport j
595595
new UniverseBuilder(aUniverse, bb.getMetaAccess(), hUniverse, hMetaAccess, HostedConfiguration.instance().createStrengthenGraphs(bb, hUniverse),
596596
bb.getUnsupportedFeatures()).build(debug);
597597

598-
if (TransitiveEffectSummaryAnalysisEngine.enabled()) {
598+
if (TesaEngine.enabled()) {
599599
/*
600600
* Fixed-point loops are started after universe building, because the initial
601601
* state for each method is computed after strengthen graphs, which is executed
602602
* in UniverseBuilder.
603603
*/
604-
TransitiveEffectSummaryAnalysisEngine.get().runFixedPointLoops(bb);
604+
TesaEngine.get().runFixedPointLoops(bb);
605605
}
606606

607607
BuildPhaseProvider.markHostedUniverseBuilt();
@@ -835,15 +835,15 @@ protected boolean runPointsToAnalysis(String imageName, OptionValues options, De
835835
HostedImageLayerBuildingSupport.singleton().getWriter().initializeExternalValues();
836836
}
837837
}
838-
if (TransitiveEffectSummaryAnalysisEngine.enabled()) {
838+
if (TesaEngine.enabled()) {
839839
/*
840840
* Seal the TESA engine, prevent more analyses from being registered.
841841
* Technically, we could currently allow registrations even during the
842842
* Feature#beforeUniverseBuilding callbacks. But we are intentionally being
843843
* stricter about registrations in case we would like to perform some TESA steps
844844
* already during or immediately after PTA in the future.
845845
*/
846-
TransitiveEffectSummaryAnalysisEngine.get().seal();
846+
TesaEngine.get().seal();
847847
}
848848
}
849849

@@ -901,8 +901,8 @@ protected boolean runPointsToAnalysis(String imageName, OptionValues options, De
901901
bb.getUnsupportedFeatures().report(bb);
902902
bb.checkUserLimitations();
903903

904-
if (TransitiveEffectSummaryAnalysisEngine.enabled()) {
905-
TransitiveEffectSummaryAnalysisEngine.get().saveCallGraph(bb);
904+
if (TesaEngine.enabled()) {
905+
TesaEngine.get().saveCallGraph(bb);
906906
}
907907

908908
bb.afterAnalysis();
@@ -1080,16 +1080,16 @@ protected void setupNativeImage(OptionValues options, Map<Method, CEntryPointDat
10801080

10811081
bb = createBigBang(debug, options, aUniverse, aMetaAccess, aProviders, annotationSubstitutions);
10821082
aUniverse.setBigBang(bb);
1083-
if (TransitiveEffectSummaryAnalysisEngine.Options.TransitiveEffectSummaryAnalysis.getValue()) {
1083+
if (TesaEngine.Options.TransitiveEffectSummaryAnalysis.getValue()) {
10841084
/*
10851085
* Tesa only works in ClosedTypeWorld. PLTGOT seems to create memory kills
10861086
* unseen by TESA.
10871087
*/
1088-
if (SubstrateOptions.ClosedTypeWorld.getValue() && !PLTGOTOptions.EnablePLTGOT.getValue()) {
1089-
TransitiveEffectSummaryAnalysisEngine engine = new TransitiveEffectSummaryAnalysisEngine();
1090-
ImageSingletons.add(TransitiveEffectSummaryAnalysisEngine.class, engine);
1088+
if (SubstrateOptions.useClosedTypeWorld() && !PLTGOTOptions.EnablePLTGOT.getValue()) {
1089+
TesaEngine engine = new TesaEngine();
1090+
ImageSingletons.add(TesaEngine.class, engine);
10911091
/* Register the engine in ImageBuildStatistics for reporting. */
1092-
ImageSingletons.add(ImageBuildStatistics.TransitiveEffectSummaryAnalysisPrinter.class, engine);
1092+
ImageSingletons.add(ImageBuildStatistics.TesaPrinter.class, engine);
10931093
}
10941094
}
10951095
if (imageLayerLoader != null) {

‎substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/ProgressReporter.java‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@
2525
package com.oracle.svm.hosted;
2626

2727
import static com.oracle.svm.hosted.ProgressReporterJsonHelper.UNAVAILABLE_METRIC;
28+
import static com.oracle.svm.hosted.analysis.tesa.TesaEngine.Options.TesaPrintToConsole;
2829

2930
import java.io.IOException;
3031
import java.io.PrintWriter;
@@ -101,7 +102,7 @@
101102
import com.oracle.svm.hosted.ProgressReporterJsonHelper.JsonMetric;
102103
import com.oracle.svm.hosted.ProgressReporterJsonHelper.ResourceUsageKey;
103104
import com.oracle.svm.hosted.analysis.tesa.AbstractTesa;
104-
import com.oracle.svm.hosted.analysis.tesa.TransitiveEffectSummaryAnalysisEngine;
105+
import com.oracle.svm.hosted.analysis.tesa.TesaEngine;
105106
import com.oracle.svm.hosted.c.codegen.CCompilerInvoker;
106107
import com.oracle.svm.hosted.image.AbstractImage.NativeImageKind;
107108
import com.oracle.svm.hosted.image.NativeImageDebugInfoStripFeature;
@@ -645,7 +646,7 @@ public void printCreationEnd(int imageFileSize, int heapObjectCount, long imageH
645646
l().a(", %s in total file size", ByteFormattingUtil.bytesToHuman(imageDiskFileSize));
646647
}
647648
l().println();
648-
if (TransitiveEffectSummaryAnalysisEngine.enabled() && TransitiveEffectSummaryAnalysisEngine.Options.TesaPrintToConsole.getValue()) {
649+
if (TesaEngine.enabled() && TesaPrintToConsole.getValue()) {
649650
printTesaStatistics();
650651
}
651652
printBreakdowns();
@@ -763,7 +764,7 @@ private void printRecommendations() {
763764
}
764765

765766
private void printTesaStatistics() {
766-
TransitiveEffectSummaryAnalysisEngine engine = TransitiveEffectSummaryAnalysisEngine.get();
767+
TesaEngine engine = TesaEngine.get();
767768

768769
/* Separate the text from the previous section. */
769770
l().printLineSeparator();

‎substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/SubstrateStrengthenGraphs.java‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,7 @@
4141
import com.oracle.svm.core.util.HostedStringDeduplication;
4242
import com.oracle.svm.core.util.VMError;
4343
import com.oracle.svm.hosted.analysis.Inflation;
44-
import com.oracle.svm.hosted.analysis.tesa.TransitiveEffectSummaryAnalysisEngine;
44+
import com.oracle.svm.hosted.analysis.tesa.TesaEngine;
4545
import com.oracle.svm.hosted.code.SubstrateCompilationDirectives;
4646
import com.oracle.svm.hosted.imagelayer.HostedImageLayerBuildingSupport;
4747
import com.oracle.svm.hosted.meta.HostedType;
@@ -66,12 +66,14 @@ public class SubstrateStrengthenGraphs extends StrengthenGraphs {
6666
private final Boolean trackDynamicAccess;
6767
private final Boolean trackJavaHomeAccess;
6868
private final Boolean trackJavaHomeAccessDetailed;
69+
private final TesaEngine tesaEngine;
6970

7071
public SubstrateStrengthenGraphs(Inflation bb, Universe converter) {
7172
super(bb, converter);
7273
trackDynamicAccess = TrackDynamicAccessEnabled.isTrackDynamicAccessEnabled();
7374
trackJavaHomeAccess = SubstrateOptions.TrackJavaHomeAccess.getValue();
7475
trackJavaHomeAccessDetailed = SubstrateOptions.TrackJavaHomeAccessDetailed.getValue();
76+
tesaEngine = TesaEngine.enabled() ? TesaEngine.get() : null;
7577
}
7678

7779
@Override
@@ -86,9 +88,9 @@ protected void postStrengthenGraphs(StructuredGraph graph, AnalysisMethod method
8688
if (trackJavaHomeAccess) {
8789
new AnalyzeJavaHomeAccessPhase(trackJavaHomeAccessDetailed, bb.getMetaAccess()).apply(graph, bb.getProviders(method));
8890
}
89-
if (TransitiveEffectSummaryAnalysisEngine.enabled()) {
91+
if (tesaEngine != null) {
9092
/* Use graphs already improved by the analysis. */
91-
TransitiveEffectSummaryAnalysisEngine.get().initializeStateForMethod(method, graph);
93+
tesaEngine.initializeStateForMethod(method, graph);
9294
}
9395
}
9496

‎substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/analysis/tesa/AbstractTesa.java‎

Lines changed: 27 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -47,12 +47,13 @@
4747
import jdk.graal.compiler.nodes.StartNode;
4848
import jdk.graal.compiler.nodes.StructuredGraph;
4949
import jdk.graal.compiler.nodes.java.ExceptionObjectNode;
50+
import jdk.graal.compiler.nodes.memory.MemoryKill;
5051

5152
/**
5253
* The base class for any Transitive Effect Summary Analysis.
5354
*
5455
* @param <T> the effect tracked by the given analysis.
55-
* @see TransitiveEffectSummaryAnalysisEngine
56+
* @see TesaEngine
5657
*/
5758
public abstract class AbstractTesa<T extends TesaEffect<T>> {
5859
/**
@@ -77,14 +78,22 @@ public abstract class AbstractTesa<T extends TesaEffect<T>> {
7778

7879
/**
7980
* Computes the initial state for the given {@code method}.
81+
* <p>
82+
* While the algorithm to compute the initial state can be arbitrary, it is advisable that it
83+
* contains at most a single pass over the {@code graph} and does not rely on implementation
84+
* details of the Graal IR. Ideally, only high-level APIs and marker interfaces should be used
85+
* to make sure that the code in {@code computeInitialState} does not became incorrect when new
86+
* Graal IR nodes are introduced. See how the {@link KilledLocationTesa#computeInitialState}
87+
* uses {@link MemoryKill#isMemoryKill(Node)} as an example.
8088
*/
8189
protected abstract T computeInitialState(AnalysisMethod method, StructuredGraph graph);
8290

8391
/**
8492
* Computes and saves the initial state of this method to be used later by the analysis.
8593
*/
8694
void initializeStateForMethod(AnalysisMethod method, StructuredGraph graph) {
87-
AnalysisError.guarantee(methodToState.put(method, computeInitialState(method, graph)) == null, "A state for method %s has already been initialized.", method);
95+
T previous = methodToState.put(method, computeInitialState(method, graph));
96+
AnalysisError.guarantee(previous == null, "A state for method %s has already been initialized.", method);
8897
}
8998

9099
/**
@@ -127,7 +136,7 @@ protected boolean shouldSkipNode(Node node) {
127136
* perform a linear sweep over the graph. However, since the overhead of the fixed-point
128137
* algorithm is reasonably low, we stick with it for now, as it is simpler.
129138
*/
130-
void runFixedPointLoop(TransitiveEffectSummaryAnalysisEngine engine, BigBang bb) {
139+
void runFixedPointLoop(TesaEngine engine, BigBang bb) {
131140
try (var _ = fixedPointLoopTimer.start()) {
132141
var scheduledMethods = TesaReverseCallGraph.getAllMethods(bb).collect(Collectors.toCollection(HashSet::new));
133142
var worklist = new ArrayDeque<>(scheduledMethods);
@@ -139,7 +148,7 @@ void runFixedPointLoop(TransitiveEffectSummaryAnalysisEngine engine, BigBang bb)
139148
long limit = ((long) scheduledMethods.size()) * scheduledMethods.size();
140149
while (!worklist.isEmpty()) {
141150
if (iterations >= limit) {
142-
if (TransitiveEffectSummaryAnalysisEngine.Options.TesaThrowOnNonTermination.getValue()) {
151+
if (TesaEngine.Options.TesaThrowOnNonTermination.getValue()) {
143152
throw AnalysisError.shouldNotReachHere(ClassUtil.getUnqualifiedName(getClass()) + ": fixed-point loop did not terminate after " + iterations + " iterations.");
144153
} else {
145154
/*
@@ -200,14 +209,14 @@ public T getState(AnalysisMethod currentMethod) {
200209
* For indirect invokes, use the {@link TesaEffect#combineEffects} over the states of all target
201210
* methods.
202211
*/
203-
public T getState(TransitiveEffectSummaryAnalysisEngine engine, Invoke invoke) {
212+
public T getState(TesaEngine engine, Invoke invoke) {
204213
var targetMethod = ((HostedMethod) invoke.callTarget().targetMethod());
205214
if (invoke.getInvokeKind().isDirect()) {
206215
return getState(targetMethod.wrapped);
207216
} else {
208217
T cummulativeState = noEffect();
209-
for (AnalysisMethod targetMethods : engine.getCallGraph().getTargetMethods(invoke, targetMethod)) {
210-
var targetState = getState(targetMethods);
218+
for (AnalysisMethod callee : engine.getCallGraph().getCallees(invoke, targetMethod)) {
219+
var targetState = getState(callee);
211220
cummulativeState = cummulativeState.combineEffects(targetState);
212221
if (cummulativeState.isAnyEffect()) {
213222
break;
@@ -220,9 +229,10 @@ public T getState(TransitiveEffectSummaryAnalysisEngine engine, Invoke invoke) {
220229
/**
221230
* Apply the results of this analysis on the given compilation {@code graph}.
222231
*/
223-
public void applyResults(TransitiveEffectSummaryAnalysisEngine engine, HostedMethod method, StructuredGraph graph) {
232+
public void applyResults(TesaEngine engine, HostedMethod method, StructuredGraph graph) {
224233
var state = getState(method.wrapped);
225234
if (hasOptimizationPotential(state)) {
235+
onOptimizableMethodDiscovered(method, state, graph);
226236
optimizableMethodsCounter.incrementAndGet();
227237
}
228238
for (Node node : graph.getNodes()) {
@@ -236,6 +246,15 @@ public void applyResults(TransitiveEffectSummaryAnalysisEngine engine, HostedMet
236246
}
237247
}
238248

249+
/**
250+
* Hook for subclasses to perform any postprocessing or correctness checks for methods found as
251+
* optimizable.
252+
*/
253+
@SuppressWarnings("unused")
254+
protected void onOptimizableMethodDiscovered(HostedMethod method, T state, StructuredGraph graph) {
255+
256+
}
257+
239258
/**
240259
* Hook for subclasses to check if the given {@code invoke} can be optimized by the given
241260
* analysis. By default, try to optimize all invokes.

‎substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/analysis/tesa/KilledLocationTesa.java‎

Lines changed: 22 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -29,11 +29,13 @@
2929
import com.oracle.graal.pointsto.meta.AnalysisMethod;
3030
import com.oracle.graal.pointsto.util.AnalysisError;
3131
import com.oracle.svm.hosted.analysis.tesa.effect.LocationEffect;
32+
import com.oracle.svm.hosted.meta.HostedMethod;
3233

3334
import jdk.graal.compiler.graph.Node;
3435
import jdk.graal.compiler.nodes.Invoke;
3536
import jdk.graal.compiler.nodes.InvokeNode;
3637
import jdk.graal.compiler.nodes.InvokeWithExceptionNode;
38+
import jdk.graal.compiler.nodes.SafepointNode;
3739
import jdk.graal.compiler.nodes.StructuredGraph;
3840
import jdk.graal.compiler.nodes.memory.MemoryKill;
3941
import jdk.graal.compiler.nodes.memory.MultiMemoryKill;
@@ -94,16 +96,33 @@ public static LocationIdentity[] extractLocationIdentities(Node node) {
9496
};
9597
}
9698

99+
@Override
100+
protected void onOptimizableMethodDiscovered(HostedMethod method, LocationEffect state, StructuredGraph graph) {
101+
assert checkNoSafepointNodes(method, state, graph);
102+
}
103+
104+
/**
105+
* Methods containing {@link SafepointNode} can kill multiple locations, which we do not track
106+
* precisely at the moment. To guarantee correctness, we verify that only methods without
107+
* safepoints have more precise killed location than {@code any}.
108+
*/
109+
private static boolean checkNoSafepointNodes(HostedMethod method, LocationEffect state, StructuredGraph graph) {
110+
for (Node node : graph.getNodes()) {
111+
AnalysisError.guarantee(!(node instanceof SafepointNode), "Method %s has a safepoint node in its graph, but its killed location is not any: %s", method.getQualifiedName(), state);
112+
}
113+
return true;
114+
}
115+
97116
@Override
98117
protected void optimizeInvoke(StructuredGraph graph, Invoke invoke, LocationEffect targetState) {
99118
switch (targetState) {
100-
case LocationEffect.Empty _ -> insertNewLocationIdentity(invoke, MemoryKill.NO_LOCATION);
101-
case LocationEffect.Single single -> insertNewLocationIdentity(invoke, single.location);
119+
case LocationEffect.Empty _ -> setKilledLocationIdentity(invoke, MemoryKill.NO_LOCATION);
120+
case LocationEffect.Single single -> setKilledLocationIdentity(invoke, single.location);
102121
default -> AnalysisError.shouldNotReachHere(targetState + " is not actionable.");
103122
}
104123
}
105124

106-
private static void insertNewLocationIdentity(Invoke invoke, LocationIdentity locationIdentity) {
125+
private static void setKilledLocationIdentity(Invoke invoke, LocationIdentity locationIdentity) {
107126
switch (invoke) {
108127
case InvokeNode invokeNode -> invokeNode.setKilledLocationIdentity(locationIdentity);
109128
case InvokeWithExceptionNode invokeWithExceptionNode -> invokeWithExceptionNode.setKilledLocationIdentity(locationIdentity);

0 commit comments

Comments
 (0)