[GR-4369] Invert branch probabilities for negated conditions.

PullRequest: graal/380
ertugrulcetin · Mar 16, 2018 · dc10860 · dc10860
2 parents 7768ba6 + 98038df
commit dc10860
Show file tree

Hide file tree

Showing 11 changed files with 50 additions and 27 deletions.
diff --git a/compiler/src/org.graalvm.compiler.graph/src/org/graalvm/compiler/graph/Node.java b/compiler/src/org.graalvm.compiler.graph/src/org/graalvm/compiler/graph/Node.java
@@ -593,7 +593,7 @@ public NodeSourcePosition getNodeSourcePosition() {
      * Set the source position to {@code sourcePosition}.
      */
     public void setNodeSourcePosition(NodeSourcePosition sourcePosition) {
-        assert sourcePosition != null || this.sourcePosition == null || this.sourcePosition.isPlaceholder();
+        assert sourcePosition != null || this.sourcePosition == null || this.sourcePosition.isPlaceholder() : "Invalid source position at node with id " + id;
         this.sourcePosition = sourcePosition;
         // assert sourcePosition == null || graph == null || graph.trackNodeSourcePosition;
     }

diff --git a/compiler/src/org.graalvm.compiler.graph/src/org/graalvm/compiler/graph/NodeClass.java b/compiler/src/org.graalvm.compiler.graph/src/org/graalvm/compiler/graph/NodeClass.java
@@ -126,7 +126,7 @@ private static <T> NodeClass<T> getUnchecked(Class<T> clazz) {
 
     public static <T> NodeClass<T> get(Class<T> clazz) {
         NodeClass<T> result = getUnchecked(clazz);
-        if (result == null) {
+        if (result == null && clazz != NODE_CLASS) {
             throw GraalError.shouldNotReachHere("TYPE field not initialized for class " + clazz.getTypeName());
         }
         return result;

diff --git a/...r/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/NodeCostDumpUtil.java b/...r/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/NodeCostDumpUtil.java
@@ -24,7 +24,6 @@
 
 import java.io.File;
 import java.io.IOException;
-import java.lang.reflect.Field;
 import java.net.URI;
 import java.net.URL;
 import java.net.URLClassLoader;
@@ -114,14 +113,9 @@ public static void main(String[] args) {
         }
         System.err.printf("Loaded %s node classes...\n", nodeClasses.size());
         List<NodeClass<?>> nc = new ArrayList<>();
-        for (Class<?> nodeClass : nodeClasses) {
-            Field f;
+        for (Class<?> c : nodeClasses) {
             try {
-                f = nodeClass.getField("TYPE");
-                f.setAccessible(true);
-                Object val = f.get(null);
-                NodeClass<?> nodeType = (NodeClass<?>) val;
-                nc.add(nodeType);
+                nc.add(NodeClass.get(c));
             } catch (Throwable t) {
                 // Silently ignore problems here
             }

diff --git a/compiler/src/org.graalvm.compiler.java/src/org/graalvm/compiler/java/BytecodeParser.java b/compiler/src/org.graalvm.compiler.java/src/org/graalvm/compiler/java/BytecodeParser.java
@@ -3179,7 +3179,7 @@ protected void genIf(ValueNode x, Condition cond, ValueNode y) {
         genIf(condition, trueSuccessor, falseSuccessor, probability);
     }
 
-    private double getProfileProbability(boolean negate) {
+    protected double getProfileProbability(boolean negate) {
         double probability;
         if (profilingInfo == null) {
             probability = 0.5;

diff --git a/compiler/src/org.graalvm.compiler.nodes/src/org/graalvm/compiler/nodes/LoopExitNode.java b/compiler/src/org.graalvm.compiler.nodes/src/org/graalvm/compiler/nodes/LoopExitNode.java
@@ -105,7 +105,9 @@ public void simplify(SimplifierTool tool) {
         Node prev = this.predecessor();
         while (tool.allUsagesAvailable() && prev instanceof BeginNode && prev.hasNoUsages()) {
             AbstractBeginNode begin = (AbstractBeginNode) prev;
-            this.setNodeSourcePosition(begin.getNodeSourcePosition());
+            if (begin.getNodeSourcePosition() != null || this.getNodeSourcePosition() == null || this.getNodeSourcePosition().isPlaceholder()) {
+                this.setNodeSourcePosition(begin.getNodeSourcePosition());
+            }
             prev = prev.predecessor();
             graph().removeFixed(begin);
         }

diff --git a/...org.graalvm.compiler.phases/src/org/graalvm/compiler/phases/contract/VerifyNodeCosts.java b/...org.graalvm.compiler.phases/src/org/graalvm/compiler/phases/contract/VerifyNodeCosts.java
@@ -22,7 +22,6 @@
  */
 package org.graalvm.compiler.phases.contract;
 
-import java.lang.reflect.Field;
 import java.lang.reflect.Modifier;
 import java.util.function.Predicate;
 
@@ -64,13 +63,8 @@ public static void verifyNodeClass(Class<?> clazz) {
     }
 
     private static NodeClass<?> getType(Class<?> c) {
-        Field f;
         try {
-            f = c.getField("TYPE");
-            f.setAccessible(true);
-            Object val = f.get(null);
-            NodeClass<?> nodeType = (NodeClass<?>) val;
-            return nodeType;
+            return NodeClass.get(c);
         } catch (Throwable t) {
             throw new VerifyPhase.VerificationError("%s.java does not specify a TYPE field.", c.getName());
         }

diff --git a/substratevm/mx.substratevm/mx_substratevm.py b/substratevm/mx.substratevm/mx_substratevm.py
@@ -593,10 +593,12 @@ def gate_sulong(native_image, tasks):
             sulong = truffle_language_ensure('llvm')
             sulong.extensions.runLLVMUnittests(functools.partial(native_junit, native_image, build_args=['--Language:llvm']))
 
-def js_image_test(binary, bench_location, name, warmup_iterations, iterations, timeout=None):
-    jsruncmd = [binary, join(bench_location, 'harness.js'), '--',
-                join(bench_location, name + '.js'), '--', '--warmup-iterations=' + str(warmup_iterations),
-                '--iterations=' + str(iterations)]
+
+def js_image_test(binary, bench_location, name, warmup_iterations, iterations, timeout=None, bin_args=None):
+    bin_args = bin_args if bin_args is not None else []
+    jsruncmd = [binary] + bin_args + [join(bench_location, 'harness.js'), '--', join(bench_location, name + '.js'),
+                                      '--', '--warmup-iterations=' + str(warmup_iterations),
+                                      '--iterations=' + str(iterations)]
     mx.log(' '.join(jsruncmd))
 
     passing = []

diff --git a/...om.oracle.graal.pointsto/src/com/oracle/graal/pointsto/results/StaticAnalysisResults.java b/...om.oracle.graal.pointsto/src/com/oracle/graal/pointsto/results/StaticAnalysisResults.java
@@ -33,7 +33,7 @@
 public class StaticAnalysisResults implements ProfilingInfo {
     public static final StaticAnalysisResults NO_RESULTS = new StaticAnalysisResults(0, null, null, null);
 
-    protected static class BytecodeEntry {
+    public static class BytecodeEntry {
         /** The bytecode index of this entry. */
         private final int bci;
 

diff --git a/...le.graal.pointsto/src/com/oracle/graal/pointsto/results/StaticAnalysisResultsBuilder.java b/...le.graal.pointsto/src/com/oracle/graal/pointsto/results/StaticAnalysisResultsBuilder.java
@@ -60,7 +60,7 @@ public class StaticAnalysisResultsBuilder {
      * The universe used to convert analysis metadata to hosted metadata, or {@code null} if no
      * conversion should be performed.
      */
-    private final Universe converter;
+    protected final Universe converter;
 
     /* Caches for JavaTypeProfile with 0, 1, or more types. */
     private final JavaTypeProfile[] types0;
@@ -122,7 +122,7 @@ public StaticAnalysisResults makeResults(AnalysisMethod method) {
                 if (typeProfile != null) {
                     ensureSize(entries, bci);
                     assert entries.get(bci) == null : "In " + method.format("%h.%n(%p)") + " a profile with bci=" + bci + " already exists: " + entries.get(bci);
-                    entries.set(bci, new BytecodeEntry(bci, typeProfile, null, null));
+                    entries.set(bci, createBytecodeEntry(method, bci, typeProfile, null, null));
                 }
             }
         }
@@ -151,7 +151,7 @@ public StaticAnalysisResults makeResults(AnalysisMethod method) {
                 if (typeProfile != null || methodProfile != null || invokeResultTypeProfile != null) {
                     ensureSize(entries, bci);
                     assert entries.get(bci) == null : "In " + method.format("%h.%n(%p)") + " a profile with bci=" + bci + " already exists: " + entries.get(bci);
-                    entries.set(bci, new BytecodeEntry(bci, typeProfile, methodProfile, invokeResultTypeProfile));
+                    entries.set(bci, createBytecodeEntry(method, bci, typeProfile, methodProfile, invokeResultTypeProfile));
                 }
             }
         }
@@ -192,6 +192,15 @@ public StaticAnalysisResults makeResults(AnalysisMethod method) {
             }
         }
 
+        return createStaticAnalysisResults(method, parameterTypeProfiles, resultTypeProfile, first);
+    }
+
+    protected BytecodeEntry createBytecodeEntry(@SuppressWarnings("unused") AnalysisMethod method, int bci, JavaTypeProfile typeProfile, JavaMethodProfile methodProfile,
+                    JavaTypeProfile invokeResultTypeProfile) {
+        return new BytecodeEntry(bci, typeProfile, methodProfile, invokeResultTypeProfile);
+    }
+
+    protected StaticAnalysisResults createStaticAnalysisResults(AnalysisMethod method, JavaTypeProfile[] parameterTypeProfiles, JavaTypeProfile resultTypeProfile, BytecodeEntry first) {
         if (parameterTypeProfiles == null && resultTypeProfile == null && first == null) {
             return StaticAnalysisResults.NO_RESULTS;
         } else {

diff --git a/substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/HostedConfiguration.java b/substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/HostedConfiguration.java
@@ -84,4 +84,8 @@ public void findAllFieldsForLayout(HostedUniverse universe, @SuppressWarnings("u
     public StaticAnalysisResultsBuilder createStaticAnalysisResultsBuilder(BigBang bigbang, HostedUniverse universe) {
         return new StaticAnalysisResultsBuilder(bigbang, universe);
     }
+
+    public boolean isUsingAOTProfiles() {
+        return false;
+    }
 }
diff --git a/...m/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/phases/SharedGraphBuilderPhase.java b/...m/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/phases/SharedGraphBuilderPhase.java
@@ -50,6 +50,7 @@
 import com.oracle.svm.core.meta.SubstrateObjectConstant;
 import com.oracle.svm.core.util.UserError;
 import com.oracle.svm.core.util.UserError.UserException;
+import com.oracle.svm.hosted.HostedConfiguration;
 import com.oracle.svm.hosted.NativeImageOptions;
 
 import jdk.vm.ci.meta.ConstantReflectionProvider;
@@ -307,6 +308,23 @@ protected void genIf(ValueNode x, Condition cond, ValueNode y) {
             super.genIf(x, cond, y);
         }
 
+        @Override
+        protected double getProfileProbability(boolean negate) {
+            double probability = super.getProfileProbability(negate);
+            if (negate && HostedConfiguration.instance().isUsingAOTProfiles()) {
+                /*
+                 * Probabilities from AOT profiles are about canonical conditions as they are coming
+                 * from Graal IR, but since `BytecodeParser` assumes that probabilities are about
+                 * original conditions, it will negate them when negating original conditions during
+                 * conversion to Graal IR. Therefore, we have to negate probabilities back to their
+                 * initial value in case of AOT profiles, as otherwise they would be applied to a
+                 * wrong branch.
+                 */
+                return 1 - probability;
+            }
+            return probability;
+        }
+
         @Override
         public MethodCallTargetNode createMethodCallTarget(InvokeKind invokeKind, ResolvedJavaMethod targetMethod, ValueNode[] args, StampPair returnStamp, JavaTypeProfile profile) {
             boolean isStatic = targetMethod.isStatic();