[GR-7838] Fix injection of branch probability

* Introduce `CanonicalCondition` that is limited to the conditions that are allowed in the Graal IR. * Restructure the code dealing with if branches in the bytecode parser to avoid inverted probabilities while canonicalinzg conditions.
Utowoo · Jan 23, 2018 · d0d0e4e · d0d0e4e
1 parent b87da48
commit d0d0e4e
Show file tree

Hide file tree

Showing 31 changed files with 366 additions and 229 deletions.
diff --git a/...irectives.test/src/org/graalvm/compiler/api/directives/test/ProbabilityDirectiveTest.java b/...irectives.test/src/org/graalvm/compiler/api/directives/test/ProbabilityDirectiveTest.java
@@ -50,6 +50,21 @@ public void testBranchProbability() {
         test("branchProbabilitySnippet", 5);
     }
 
+    public static int branchProbabilitySnippet2(int arg) {
+        if (!GraalDirectives.injectBranchProbability(0.125, arg <= 0)) {
+            GraalDirectives.controlFlowAnchor(); // prevent removal of the if
+            return 2;
+        } else {
+            GraalDirectives.controlFlowAnchor(); // prevent removal of the if
+            return 1;
+        }
+    }
+
+    @Test
+    public void testBranchProbability2() {
+        test("branchProbabilitySnippet2", 5);
+    }
+
     @Override
     protected boolean checkLowTierGraph(StructuredGraph graph) {
         NodeIterable<IfNode> ifNodes = graph.getNodes(IfNode.TYPE);

diff --git a/....graalvm.compiler.core.amd64/src/org/graalvm/compiler/core/amd64/AMD64NodeMatchRules.java b/....graalvm.compiler.core.amd64/src/org/graalvm/compiler/core/amd64/AMD64NodeMatchRules.java
@@ -44,6 +44,7 @@
 import org.graalvm.compiler.asm.amd64.AMD64Assembler.SSEOp;
 import org.graalvm.compiler.core.common.LIRKind;
 import org.graalvm.compiler.core.common.NumUtil;
+import org.graalvm.compiler.core.common.calc.CanonicalCondition;
 import org.graalvm.compiler.core.common.calc.Condition;
 import org.graalvm.compiler.core.gen.NodeLIRBuilder;
 import org.graalvm.compiler.core.gen.NodeMatchRules;
@@ -128,7 +129,7 @@ protected OperandSize getMemorySize(LIRLowerableAccess access) {
     }
 
     protected ComplexMatchResult emitCompareBranchMemory(IfNode ifNode, CompareNode compare, ValueNode value, LIRLowerableAccess access) {
-        Condition cond = compare.condition();
+        Condition cond = compare.condition().asCondition();
         AMD64Kind kind = getMemoryKind(access);
         boolean matchedAsConstant = false; // For assertion checking
 
@@ -303,7 +304,7 @@ public ComplexMatchResult ifCompareMemory(IfNode root, CompareNode compare, Valu
     @MatchRule("(If (FloatEquals=compare value ValueCompareAndSwap=cas))")
     @MatchRule("(If (IntegerEquals=compare value ValueCompareAndSwap=cas))")
     public ComplexMatchResult ifCompareValueCas(IfNode root, CompareNode compare, ValueNode value, ValueCompareAndSwapNode cas) {
-        assert compare.condition() == Condition.EQ;
+        assert compare.condition() == CanonicalCondition.EQ;
         if (value == cas.getExpectedValue() && cas.usages().count() == 1) {
             return builder -> {
                 LIRKind kind = getLirKind(cas);
@@ -326,7 +327,7 @@ public ComplexMatchResult ifCompareValueCas(IfNode root, CompareNode compare, Va
     @MatchRule("(If (IntegerEquals=compare value LogicCompareAndSwap=cas))")
     public ComplexMatchResult ifCompareLogicCas(IfNode root, CompareNode compare, ValueNode value, LogicCompareAndSwapNode cas) {
         JavaConstant constant = value.asJavaConstant();
-        assert compare.condition() == Condition.EQ;
+        assert compare.condition() == CanonicalCondition.EQ;
         if (constant != null && cas.usages().count() == 1) {
             long constantValue = constant.asLong();
             boolean successIsTrue;

diff --git a/...vm.compiler.core.common/src/org/graalvm/compiler/core/common/calc/CanonicalCondition.java b/...vm.compiler.core.common/src/org/graalvm/compiler/core/common/calc/CanonicalCondition.java
@@ -0,0 +1,52 @@
+/*
+ * Copyright (c) 2009, 2015, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+package org.graalvm.compiler.core.common.calc;
+
+import jdk.vm.ci.meta.Constant;
+import jdk.vm.ci.meta.ConstantReflectionProvider;
+import jdk.vm.ci.meta.PrimitiveConstant;
+
+public enum CanonicalCondition {
+    EQ(Condition.EQ),
+    LT(Condition.LT),
+    BT(Condition.BT);
+
+    private final Condition condition;
+
+    CanonicalCondition(Condition condition) {
+        assert condition.isCanonical();
+        this.condition = condition;
+    }
+
+    public Condition asCondition() {
+        return condition;
+    }
+
+    public boolean foldCondition(Constant lt, Constant rt, ConstantReflectionProvider constantReflection, boolean unorderedIsTrue) {
+        return asCondition().foldCondition(lt, rt, constantReflection, unorderedIsTrue);
+    }
+
+    public boolean foldCondition(PrimitiveConstant lp, PrimitiveConstant rp, boolean unorderedIsTrue) {
+        return asCondition().foldCondition(lp, rp, unorderedIsTrue);
+    }
+}
diff --git a/...org.graalvm.compiler.core.common/src/org/graalvm/compiler/core/common/calc/Condition.java b/...org.graalvm.compiler.core.common/src/org/graalvm/compiler/core/common/calc/Condition.java
@@ -115,6 +115,55 @@ public boolean check(int left, int right) {
         throw new IllegalArgumentException(this.toString());
     }
 
+    public static final class CanonicalizedCondition {
+        private final CanonicalCondition canonicalCondition;
+        private final boolean mirror;
+        private final boolean negate;
+
+        private CanonicalizedCondition(CanonicalCondition canonicalCondition, boolean mirror, boolean negate) {
+            this.canonicalCondition = canonicalCondition;
+            this.mirror = mirror;
+            this.negate = negate;
+        }
+
+        public CanonicalCondition getCanonicalCondition() {
+            return canonicalCondition;
+        }
+
+        public boolean mustMirror() {
+            return mirror;
+        }
+
+        public boolean mustNegate() {
+            return negate;
+        }
+    }
+
+    public CanonicalizedCondition canonicalize() {
+        CanonicalCondition canonicalCondition;
+        switch (this) {
+            case EQ:
+            case NE:
+                canonicalCondition = CanonicalCondition.EQ;
+                break;
+            case LT:
+            case LE:
+            case GT:
+            case GE:
+                canonicalCondition = CanonicalCondition.LT;
+                break;
+            case BT:
+            case BE:
+            case AT:
+            case AE:
+                canonicalCondition = CanonicalCondition.BT;
+                break;
+            default:
+                throw new IllegalArgumentException(this.toString());
+        }
+        return new CanonicalizedCondition(canonicalCondition, canonicalMirror(), canonicalNegate());
+    }
+
     /**
      * Given a condition and its negation, this method returns true for one of the two and false for
      * the other one. This can be used to keep comparisons in a canonical form.
@@ -151,7 +200,7 @@ public boolean isCanonical() {
      * Returns true if the condition needs to be mirrored to get to a canonical condition. The
      * result of the mirroring operation might still need to be negated to achieve a canonical form.
      */
-    public boolean canonicalMirror() {
+    private boolean canonicalMirror() {
         switch (this) {
             case EQ:
                 return false;
@@ -181,7 +230,7 @@ public boolean canonicalMirror() {
      * Returns true if the condition needs to be negated to get to a canonical condition. The result
      * of the negation might still need to be mirrored to achieve a canonical form.
      */
-    public boolean canonicalNegate() {
+    private boolean canonicalNegate() {
         switch (this) {
             case EQ:
                 return false;

diff --git a/....graalvm.compiler.core.sparc/src/org/graalvm/compiler/core/sparc/SPARCNodeMatchRules.java b/....graalvm.compiler.core.sparc/src/org/graalvm/compiler/core/sparc/SPARCNodeMatchRules.java
@@ -29,6 +29,7 @@
 import static jdk.vm.ci.sparc.SPARCKind.XWORD;
 
 import org.graalvm.compiler.core.common.LIRKind;
+import org.graalvm.compiler.core.common.calc.CanonicalCondition;
 import org.graalvm.compiler.core.common.calc.Condition;
 import org.graalvm.compiler.core.gen.NodeMatchRules;
 import org.graalvm.compiler.core.match.ComplexMatchResult;
@@ -147,7 +148,7 @@ public ComplexMatchResult zeroExtend(ZeroExtendNode root, Access access) {
     @MatchRule("(If (IntegerEquals=compare value LogicCompareAndSwap=cas))")
     public ComplexMatchResult ifCompareLogicCas(IfNode root, CompareNode compare, ValueNode value, LogicCompareAndSwapNode cas) {
         JavaConstant constant = value.asJavaConstant();
-        assert compare.condition() == Condition.EQ;
+        assert compare.condition() == CanonicalCondition.EQ;
         if (constant != null && cas.usages().count() == 1) {
             long constantValue = constant.asLong();
             boolean successIsTrue;

diff --git a/compiler/src/org.graalvm.compiler.core/src/org/graalvm/compiler/core/gen/NodeLIRBuilder.java b/compiler/src/org.graalvm.compiler.core/src/org/graalvm/compiler/core/gen/NodeLIRBuilder.java
@@ -538,7 +538,8 @@ private void emitNullCheckBranch(IsNullNode node, LabelRef trueSuccessor, LabelR
 
     public void emitCompareBranch(CompareNode compare, LabelRef trueSuccessor, LabelRef falseSuccessor, double trueSuccessorProbability) {
         PlatformKind kind = gen.getLIRKind(compare.getX().stamp(NodeView.DEFAULT)).getPlatformKind();
-        gen.emitCompareBranch(kind, operand(compare.getX()), operand(compare.getY()), compare.condition(), compare.unorderedIsTrue(), trueSuccessor, falseSuccessor, trueSuccessorProbability);
+        gen.emitCompareBranch(kind, operand(compare.getX()), operand(compare.getY()), compare.condition().asCondition(), compare.unorderedIsTrue(), trueSuccessor, falseSuccessor,
+                        trueSuccessorProbability);
     }
 
     public void emitIntegerTestBranch(IntegerTestNode test, LabelRef trueSuccessor, LabelRef falseSuccessor, double trueSuccessorProbability) {
@@ -566,7 +567,7 @@ public Variable emitConditional(LogicNode node, Value trueValue, Value falseValu
         } else if (node instanceof CompareNode) {
             CompareNode compare = (CompareNode) node;
             PlatformKind kind = gen.getLIRKind(compare.getX().stamp(NodeView.DEFAULT)).getPlatformKind();
-            return gen.emitConditionalMove(kind, operand(compare.getX()), operand(compare.getY()), compare.condition(), compare.unorderedIsTrue(), trueValue, falseValue);
+            return gen.emitConditionalMove(kind, operand(compare.getX()), operand(compare.getY()), compare.condition().asCondition(), compare.unorderedIsTrue(), trueValue, falseValue);
         } else if (node instanceof LogicConstantNode) {
             return gen.emitMove(((LogicConstantNode) node).getValue() ? trueValue : falseValue);
         } else if (node instanceof IntegerTestNode) {

diff --git a/...aalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/replacements/ClassGetHubNode.java b/...aalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/replacements/ClassGetHubNode.java
@@ -25,10 +25,7 @@
 import static org.graalvm.compiler.nodeinfo.NodeCycles.CYCLES_1;
 import static org.graalvm.compiler.nodeinfo.NodeSize.SIZE_1;
 
-import jdk.vm.ci.meta.JavaKind;
-import jdk.vm.ci.meta.ResolvedJavaMethod;
-
-import org.graalvm.compiler.core.common.calc.Condition;
+import org.graalvm.compiler.core.common.calc.CanonicalCondition;
 import org.graalvm.compiler.core.common.type.Stamp;
 import org.graalvm.compiler.graph.Node;
 import org.graalvm.compiler.graph.NodeClass;
@@ -56,7 +53,9 @@
 import jdk.vm.ci.meta.Constant;
 import jdk.vm.ci.meta.ConstantReflectionProvider;
 import jdk.vm.ci.meta.JavaConstant;
+import jdk.vm.ci.meta.JavaKind;
 import jdk.vm.ci.meta.MetaAccessProvider;
+import jdk.vm.ci.meta.ResolvedJavaMethod;
 import jdk.vm.ci.meta.ResolvedJavaType;
 
 /**
@@ -169,8 +168,8 @@ public boolean mayNullCheckSkipConversion() {
     }
 
     @Override
-    public boolean preservesOrder(Condition op, Constant value, ConstantReflectionProvider constantReflection) {
-        assert op == Condition.EQ || op == Condition.NE;
+    public boolean preservesOrder(CanonicalCondition op, Constant value, ConstantReflectionProvider constantReflection) {
+        assert op == CanonicalCondition.EQ;
         ResolvedJavaType exactType = constantReflection.asJavaType(value);
         return !exactType.isPrimitive();
     }