[GR-48430] Restrict movement of control flow dependent guards.

PullRequest: graal/15573

compiler/src/jdk.graal.compiler.test/src/jdk/graal/compiler/core/test/ConditionalEliminationTest11.java

@@ -126,7 +126,7 @@ public void test5() {
 
     public static int test6Snippet(int a) {
         if ((a & 8) != 0) {
-            GraalDirectives.deoptimize();
+            GraalDirectives.deoptimizeAndInvalidate();
         }
         if ((a & 15) != 15) {
             GraalDirectives.deoptimize();
@@ -135,8 +135,11 @@ public static int test6Snippet(int a) {
     }
 
     public static int reference6Snippet(int a) {
+        /*
+         * first guard needs higher priority than second guard, otherwise code folds to second guard
+         */
         if ((a & 8) != 0) {
-            GraalDirectives.deoptimize();
+            GraalDirectives.deoptimizeAndInvalidate();
         }
         GraalDirectives.deoptimize();
         return 0;

compiler/src/jdk.graal.compiler.test/src/jdk/graal/compiler/core/test/ConditionalEliminationTest9.java

@@ -27,8 +27,6 @@
 import jdk.graal.compiler.phases.common.ConditionalEliminationPhase;
 import org.junit.Test;
 
-import jdk.graal.compiler.api.directives.GraalDirectives;
-
 /**
  * Collection of tests for {@link ConditionalEliminationPhase} including those that triggered bugs
  * in this phase.
@@ -40,7 +38,7 @@ public class ConditionalEliminationTest9 extends ConditionalEliminationTestBase
     @SuppressWarnings("all")
     public static int referenceSnippet(int a) {
         if (a == 0) {
-            GraalDirectives.deoptimize();
+            return 1;
         }
         return 0;
     }
@@ -54,10 +52,10 @@ public void test1() {
     public static int test1Snippet(int a) {
         if (a == 0) {
             if (a == 0) {
-                GraalDirectives.deoptimize();
+                return 1;
             }
             if (a == 0) {
-                GraalDirectives.deoptimize();
+                return 2;
             }
         }
         return 0;

compiler/src/jdk.graal.compiler.test/src/jdk/graal/compiler/core/test/GraalCompilerTest.java

@@ -136,6 +136,7 @@
 import jdk.graal.compiler.printer.GraalDebugHandlersFactory;
 import jdk.graal.compiler.runtime.RuntimeProvider;
 import jdk.graal.compiler.test.GraalTest;
+import jdk.graal.compiler.loop.phases.SpeculativeGuardMovementPhase;
 import org.junit.After;
 import org.junit.Assert;
 import org.junit.BeforeClass;
@@ -283,6 +284,8 @@ protected Suites createSuites(OptionValues opts) {
             ret.getHighTier().removeSubTypePhases(Speculative.class);
             ret.getMidTier().removeSubTypePhases(Speculative.class);
             ret.getLowTier().removeSubTypePhases(Speculative.class);
+            // remove after GR-49600 is resolved:
+            ret.getMidTier().replaceAllPhases(SpeculativeGuardMovementPhase.class, () -> new SpeculativeGuardMovementPhase(CanonicalizerPhase.create(), false, false));
         }
 
         ListIterator<BasePhase<? super HighTierContext>> iter = ret.getHighTier().findPhase(ConvertDeoptimizeToGuardPhase.class, true);

compiler/src/jdk.graal.compiler.test/src/jdk/graal/compiler/core/test/HashCodeTest.java

@@ -24,13 +24,13 @@
  */
 package jdk.graal.compiler.core.test;
 
-import jdk.graal.compiler.core.phases.HighTier;
-import jdk.graal.compiler.core.phases.MidTier;
 import jdk.graal.compiler.nodes.InvokeNode;
 import jdk.graal.compiler.nodes.StructuredGraph;
 import jdk.graal.compiler.options.OptionValues;
 import jdk.graal.compiler.phases.OptimisticOptimizations;
 import jdk.graal.compiler.phases.tiers.MidTierContext;
+import jdk.graal.compiler.phases.tiers.Suites;
+
 import org.junit.Assert;
 import org.junit.Test;
 
@@ -120,8 +120,9 @@ public void test06() {
     private StructuredGraph buildGraphAfterMidTier(String name) {
         StructuredGraph g = parseForCompile(getResolvedJavaMethod(name));
         OptionValues options = getInitialOptions();
-        new HighTier(options).apply(g, getDefaultHighTierContext());
-        new MidTier(options).apply(g, new MidTierContext(getProviders(), getTargetProvider(), OptimisticOptimizations.ALL, g.getProfilingInfo()));
+        Suites suites = createSuites(options);
+        suites.getHighTier().apply(g, getDefaultHighTierContext());
+        suites.getMidTier().apply(g, new MidTierContext(getProviders(), getTargetProvider(), OptimisticOptimizations.ALL, g.getProfilingInfo()));
         return g;
     }
 

compiler/src/jdk.graal.compiler.test/src/jdk/graal/compiler/hotspot/test/Base64SubstitutionsTest.java

@@ -38,9 +38,6 @@
 import java.util.Set;
 import java.util.function.Supplier;
 import java.util.stream.Stream;
-
-import jdk.graal.compiler.core.common.GraalOptions;
-import jdk.graal.compiler.options.OptionValues;
 import org.junit.Assert;
 import org.junit.Test;
 
@@ -141,20 +138,6 @@ public void testEncodeByteArray1() {
         }
     }
 
-    /**
-     * The test at
-     * https://github.com/openjdk/jdk/blob/f2922682688a40529df269e1551246ac8da5d7ee/src/java.base/share/classes/java/util/Base64.java#L878
-     * is converted to a guard and (too eagerly) floated outside the loop, leading to deopt even
-     * when the input byte array has a correct ending.
-     *
-     * GR-48430.
-     */
-    private static OptionValues workaroundTooEagerDeopt() {
-        return new OptionValues(getInitialOptions(),
-                        GraalOptions.OptConvertDeoptsToGuards, false,
-                        GraalOptions.SpeculativeGuardMovement, false);
-    }
-
     /**
      * Tests {@link Encoder#encode(byte[], byte[])}.
      */
@@ -297,15 +280,14 @@ public void testDecodeByteArray1() {
      */
     @Test
     public void testDecodeByteArray2() {
-        OptionValues options = workaroundTooEagerDeopt();
         ResolvedJavaMethod m = getResolvedJavaMethod(Decoder.class, "decode", byte[].class, byte[].class);
         for (DecoderTestCase tc : getDecoders()) {
             for (int i = 0; i < PLAIN_TEXT_BYTES.length; i++) {
                 byte[] srcBytes = tc.encoded[i];
                 // JDK-8273108: Test for output buffer overrun
                 byte[] suffix = {0, (byte) 167};
                 ByteArraySupplier bas = new ByteArraySupplier(srcBytes.length, suffix);
-                test(options, m, tc.decoder, srcBytes, bas);
+                test(m, tc.decoder, srcBytes, bas);
                 Assert.assertEquals(bas.supplied.size(), 2);
                 byte[] expect = Arrays.copyOfRange(bas.supplied.get(0), 0, srcBytes.length);
                 byte[] actual = Arrays.copyOfRange(bas.supplied.get(1), 0, srcBytes.length);
@@ -328,12 +310,11 @@ public static byte[] decodeByteBufferSnippet(Decoder decoder, ByteBuffer srcBuf)
      */
     @Test
     public void testDecodeByteBuffer() {
-        OptionValues options = workaroundTooEagerDeopt();
         for (DecoderTestCase tc : getDecoders()) {
             for (int i = 0; i < PLAIN_TEXT_BYTES.length; i++) {
                 byte[] srcBytes = tc.encoded[i];
                 ByteBuffer srcBuf = ByteBuffer.wrap(srcBytes);
-                test(options, "decodeByteBufferSnippet", tc.decoder, srcBuf);
+                test("decodeByteBufferSnippet", tc.decoder, srcBuf);
             }
         }
     }

compiler/src/jdk.graal.compiler.test/src/jdk/graal/compiler/nodes/test/IfNodeCanonicalizationTest.java

@@ -24,7 +24,6 @@
  */
 package jdk.graal.compiler.nodes.test;
 
-import jdk.graal.compiler.api.directives.GraalDirectives;
 import jdk.graal.compiler.core.test.GraalCompilerTest;
 import jdk.graal.compiler.graph.Node;
 import jdk.graal.compiler.loop.phases.ConvertDeoptimizeToGuardPhase;
@@ -123,10 +122,10 @@ public void test4() {
     public boolean testSnippet4(int a, int[] limit) {
         int l = limit.length;
         if (a < 0) {
-            GraalDirectives.deoptimize();
+            return false;
         }
         if (a >= l) {
-            GraalDirectives.deoptimize();
+            return false;
         }
         return true;
     }
@@ -140,10 +139,10 @@ public void test5() {
     public boolean testSnippet5(int a, int[] limit) {
         int l = limit.length;
         if (a >= l) {
-            GraalDirectives.deoptimize();
+            return false;
         }
         if (a < 0) {
-            GraalDirectives.deoptimize();
+            return false;
         }
         return true;
     }

compiler/src/jdk.graal.compiler.test/src/jdk/graal/compiler/truffle/test/TransferToInterpreterTest.java

@@ -90,7 +90,8 @@ public Object execute(VirtualFrame frame) {
             int x = (int) frame.getArguments()[0];
             if (x == 0) {
                 CompilerDirectives.transferToInterpreter();
-            } else {
+            }
+            if (x == 1) {
                 CompilerDirectives.transferToInterpreterAndInvalidate();
             }
             return null;

compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/hotspot/stubs/Stub.java

@@ -61,6 +61,8 @@
 import jdk.graal.compiler.phases.OptimisticOptimizations;
 import jdk.graal.compiler.phases.PhaseSuite;
 import jdk.graal.compiler.phases.Speculative;
+import jdk.graal.compiler.loop.phases.SpeculativeGuardMovementPhase;
+import jdk.graal.compiler.phases.common.CanonicalizerPhase;
 import jdk.graal.compiler.phases.tiers.HighTierContext;
 import jdk.graal.compiler.phases.tiers.Suites;
 import jdk.graal.compiler.printer.GraalDebugHandlersFactory;
@@ -321,6 +323,8 @@ protected Suites createSuites() {
 
         defaultSuites.getMidTier().removeSubTypePhases(Speculative.class);
         defaultSuites.getLowTier().removeSubTypePhases(Speculative.class);
+        // remove after GR-49600 is resolved:
+        defaultSuites.getMidTier().replaceAllPhases(SpeculativeGuardMovementPhase.class, () -> new SpeculativeGuardMovementPhase(CanonicalizerPhase.create(), false, false));
 
         return new Suites(emptyHighTier, defaultSuites.getMidTier(), defaultSuites.getLowTier());
     }

compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/loop/phases/ConvertDeoptimizeToGuardPhase.java

@@ -110,9 +110,6 @@ protected void run(final StructuredGraph graph, final CoreProviders context) {
 
         for (DeoptimizeNode d : graph.getNodes(DeoptimizeNode.TYPE)) {
             assert d.isAlive();
-            if (!d.canFloat()) {
-                continue;
-            }
             try (DebugCloseable closable = d.withNodeSourcePosition()) {
                 propagateFixed(d, d, context, lazyLoops);
             }

compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/loop/phases/SpeculativeGuardMovementPhase.java

@@ -122,8 +122,19 @@
  */
 public class SpeculativeGuardMovementPhase extends PostRunCanonicalizationPhase<MidTierContext> implements FloatingGuardPhase {
 
+    private final boolean ignoreFrequency;
+    private final boolean requireSpeculationLog;
+
     public SpeculativeGuardMovementPhase(CanonicalizerPhase canonicalizer) {
         super(canonicalizer);
+        this.ignoreFrequency = false;
+        this.requireSpeculationLog = true;
+    }
+
+    public SpeculativeGuardMovementPhase(CanonicalizerPhase canonicalizer, boolean ignoreFrequency, boolean requireSpeculationLog) {
+        super(canonicalizer);
+        this.ignoreFrequency = ignoreFrequency;
+        this.requireSpeculationLog = requireSpeculationLog;
     }
 
     @Override
@@ -158,7 +169,7 @@ protected void run(StructuredGraph graph, MidTierContext context) {
                 }
                 LoopsData loops = context.getLoopsDataProvider().getLoopsData(graph);
                 loops.detectCountedLoops();
-                iterate = performSpeculativeGuardMovement(context, graph, loops);
+                iterate = performSpeculativeGuardMovement(context, graph, loops, ignoreFrequency, requireSpeculationLog);
             }
             if (change.getNodes().isEmpty() || !iterate) {
                 break;
@@ -174,20 +185,21 @@ public void updateGraphState(GraphState graphState) {
     }
 
     public static boolean performSpeculativeGuardMovement(MidTierContext context, StructuredGraph graph, LoopsData loops) {
-        return performSpeculativeGuardMovement(context, graph, loops, null, false);
+        return performSpeculativeGuardMovement(context, graph, loops, null, false, true);
     }
 
-    public static boolean performSpeculativeGuardMovement(MidTierContext context, StructuredGraph graph, LoopsData loops, boolean ignoreFrequency) {
-        return performSpeculativeGuardMovement(context, graph, loops, null, ignoreFrequency);
+    public static boolean performSpeculativeGuardMovement(MidTierContext context, StructuredGraph graph, LoopsData loops, boolean ignoreFrequency, boolean requireSpeculationLog) {
+        return performSpeculativeGuardMovement(context, graph, loops, null, ignoreFrequency, requireSpeculationLog);
     }
 
     public static boolean performSpeculativeGuardMovement(MidTierContext context, StructuredGraph graph, LoopsData loops, NodeBitMap toProcess) {
-        return performSpeculativeGuardMovement(context, graph, loops, toProcess, false);
+        return performSpeculativeGuardMovement(context, graph, loops, toProcess, false, true);
     }
 
-    public static boolean performSpeculativeGuardMovement(MidTierContext context, StructuredGraph graph, LoopsData loops, NodeBitMap toProcess, boolean ignoreFrequency) {
+    public static boolean performSpeculativeGuardMovement(MidTierContext context, StructuredGraph graph, LoopsData loops, NodeBitMap toProcess, boolean ignoreFrequency,
+                    boolean requireSpeculationLog) {
         SpeculativeGuardMovement spec = new SpeculativeGuardMovement(loops, graph.createNodeMap(), graph, context.getProfilingInfo(), graph.getSpeculationLog(), toProcess,
-                        ignoreFrequency);
+                        ignoreFrequency, requireSpeculationLog);
         spec.run();
         return spec.iterate;
     }
@@ -204,11 +216,12 @@ private static class SpeculativeGuardMovement implements Runnable {
         private final NodeBitMap toProcess;
 
         SpeculativeGuardMovement(LoopsData loops, NodeMap<HIRBlock> earliestCache, StructuredGraph graph, ProfilingInfo profilingInfo, SpeculationLog speculationLog, NodeBitMap toProcess,
-                        boolean ignoreFrequency) {
+                        boolean ignoreFrequency, boolean requireSpeculationLog) {
             this.loops = loops;
             this.earliestCache = earliestCache;
             this.graph = graph;
             this.profilingInfo = profilingInfo;
+            GraalError.guarantee(requireSpeculationLog ? speculationLog != null : true, "Graph has no speculation log attached: %s", graph);
             this.speculationLog = speculationLog;
             this.toProcess = toProcess;
             this.ignoreFrequency = ignoreFrequency;

compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/nodes/DeoptimizeNode.java

@@ -138,7 +138,8 @@ public boolean canFloat() {
      * high and would be executed under the wrong conditions.
      */
     public static boolean canFloat(DeoptimizationReason reason, DeoptimizationAction action) {
-        return action != DeoptimizationAction.None && reason != DeoptimizationReason.Unresolved;
+        return action != DeoptimizationAction.None && reason != DeoptimizationReason.Unresolved && reason != DeoptimizationReason.NotCompiledExceptionHandler &&
+                        reason != DeoptimizationReason.UnreachedCode;
     }
 
     @NodeIntrinsic

compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/nodes/loop/CountedLoopInfo.java

@@ -35,6 +35,7 @@
 import jdk.graal.compiler.graph.Node;
 import jdk.graal.compiler.nodes.AbstractBeginNode;
 import jdk.graal.compiler.nodes.ConstantNode;
+import jdk.graal.compiler.nodes.FixedGuardNode;
 import jdk.graal.compiler.nodes.GuardNode;
 import jdk.graal.compiler.nodes.IfNode;
 import jdk.graal.compiler.nodes.LogicConstantNode;
@@ -310,14 +311,23 @@ private ValueNode findOrCreatePositivePi(LogicNode noEntryCheck, ValueNode div,
         }
         HIRBlock loopBlock = cfg.blockFor(loop.loopBegin());
         for (Node checkUsage : noEntryCheck.usages()) {
-            if (checkUsage instanceof IfNode) {
-                IfNode ifCheck = (IfNode) checkUsage;
-                if (cfg.getNodeToBlock().isNew(ifCheck.falseSuccessor())) {
+            ValueNode candidateCheck = null;
+            if (checkUsage instanceof IfNode ifCheck) {
+                candidateCheck = ifCheck.falseSuccessor();
+            } else if (checkUsage instanceof FixedGuardNode guard) {
+                if (!guard.isNegated()) {
                     continue;
                 }
-                if (cfg.blockFor(ifCheck.falseSuccessor()).dominates(loopBlock)) {
-                    return graph.addOrUniqueWithInputs(PiNode.create(div, positiveIntStamp.improveWith(div.stamp(NodeView.DEFAULT)), ifCheck.falseSuccessor()));
-                }
+                candidateCheck = guard;
+            } else {
+                continue;
+            }
+
+            if (cfg.getNodeToBlock().isNew(candidateCheck)) {
+                continue;
+            }
+            if (cfg.blockFor(candidateCheck).dominates(loopBlock)) {
+                return graph.addOrUniqueWithInputs(PiNode.create(div, positiveIntStamp.improveWith(div.stamp(NodeView.DEFAULT)), candidateCheck));
             }
         }
         return null;

compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/truffle/hotspot/HotSpotTruffleCompilerImpl.java

@@ -61,6 +61,7 @@
 import jdk.graal.compiler.lir.asm.CompilationResultBuilderFactory;
 import jdk.graal.compiler.lir.asm.EntryPointDecorator;
 import jdk.graal.compiler.lir.phases.LIRSuites;
+import jdk.graal.compiler.loop.phases.SpeculativeGuardMovementPhase;
 import jdk.graal.compiler.nodes.StructuredGraph;
 import jdk.graal.compiler.nodes.StructuredGraph.AllowAssumptions;
 import jdk.graal.compiler.nodes.graphbuilderconf.GraphBuilderConfiguration;
@@ -74,6 +75,7 @@
 import jdk.graal.compiler.phases.PhaseSuite;
 import jdk.graal.compiler.phases.Speculative;
 import jdk.graal.compiler.phases.common.AbstractInliningPhase;
+import jdk.graal.compiler.phases.common.CanonicalizerPhase;
 import jdk.graal.compiler.phases.tiers.CompilerConfiguration;
 import jdk.graal.compiler.phases.tiers.HighTierContext;
 import jdk.graal.compiler.phases.tiers.Suites;
@@ -414,6 +416,8 @@ private static void removeSpeculativePhases(Suites suites) {
         suites.getHighTier().removeSubTypePhases(Speculative.class);
         suites.getMidTier().removeSubTypePhases(Speculative.class);
         suites.getLowTier().removeSubTypePhases(Speculative.class);
+        // remove after GR-49600 is resolved:
+        suites.getMidTier().replaceAllPhases(SpeculativeGuardMovementPhase.class, () -> new SpeculativeGuardMovementPhase(CanonicalizerPhase.create(), false, false));
     }
 
     @Override