[GR-59842] Change loop invariant reassociation to use phis last.

PullRequest: graal/19590

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

@@ -25,17 +25,18 @@
  */
 package jdk.graal.compiler.core.test;
 
+import java.util.List;
+import java.util.Random;
+
+import org.junit.Test;
+
 import jdk.graal.compiler.api.directives.GraalDirectives;
 import jdk.graal.compiler.graph.Node;
 import jdk.graal.compiler.graph.iterators.FilteredNodeIterable;
 import jdk.graal.compiler.nodes.StructuredGraph;
 import jdk.graal.compiler.nodes.StructuredGraph.AllowAssumptions;
 import jdk.graal.compiler.nodes.calc.BinaryArithmeticNode;
 import jdk.graal.compiler.phases.common.ReassociationPhase;
-import org.junit.Test;
-
-import java.util.List;
-import java.util.Random;
 
 public class ReassociationTest extends GraalCompilerTest {
 
@@ -429,7 +430,7 @@ public static int testLoopSnippet2() {
 
     public static int refLoopSnippet2() {
         for (int i = 0; i < LENGTH; i++) {
-            arr[i] = i * i * 8;
+            arr[i] = i * 8 * i;
             GraalDirectives.controlFlowAnchor();
         }
         return arr[100];
@@ -446,8 +447,8 @@ private void testReassociateInvariant(String testMethod, String refMethod) {
     }
 
     private void testFinalGraph(String testMethod, String refMethod) {
-        StructuredGraph expected = getFinalGraph(testMethod);
-        StructuredGraph actual = getFinalGraph(refMethod);
+        StructuredGraph actual = getFinalGraph(testMethod);
+        StructuredGraph expected = getFinalGraph(refMethod);
         assertEquals(expected, actual);
     }
 

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

@@ -251,18 +251,43 @@ public boolean apply(Node n) {
         }
     }
 
+    /**
+     * Reassociates loop invariants by pushing loop variant operands further down the operand tree.
+     *
+     * <pre>
+     *    inv2  var        inv1  inv2
+     *       \  /             \  /
+     * inv1   +     =>   var   +
+     *    \  /             \  /
+     *     +                +
+     * </pre>
+     *
+     * Also ensures that loop phis are pushed down the furthest (i.e., used as late as possible) to
+     * avoid long dependency chains on register level when calculating backedge values:
+     *
+     * <pre>
+     *     inv  phi        inv   var
+     *       \  /             \  /
+     *  var   +     =>   phi   +
+     *    \  /             \  /
+     *     +                +
+     * </pre>
+     */
     public boolean reassociateInvariants() {
         int count = 0;
         StructuredGraph graph = loopBegin().graph();
         InvariantPredicate invariant = new InvariantPredicate();
         NodeBitMap newLoopNodes = graph.createNodeBitMap();
+        var phis = loopBegin().phis();
         for (BinaryArithmeticNode<?> binary : whole().nodes().filter(BinaryArithmeticNode.class)) {
             if (!binary.mayReassociate()) {
                 continue;
             }
-            ValueNode result = BinaryArithmeticNode.reassociateMatchedValues(binary, invariant, binary.getX(), binary.getY(), NodeView.DEFAULT);
+            // pushing down loop variants will associate loop invariants at the "top"
+            ValueNode result = BinaryArithmeticNode.reassociateUnmatchedValues(binary, n -> !invariant.apply(n), NodeView.DEFAULT);
             if (result == binary) {
-                result = BinaryArithmeticNode.reassociateUnmatchedValues(binary, invariant, NodeView.DEFAULT);
+                // use loop phis as late as possible to shorten the register dependency chains
+                result = BinaryArithmeticNode.reassociateUnmatchedValues(binary, n -> n instanceof PhiNode phi && phis.contains(phi), NodeView.DEFAULT);
             }
             if (result != binary) {
                 if (!result.isAlive()) {

compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/phases/common/ReassociationPhase.java

@@ -79,17 +79,25 @@ protected void run(StructuredGraph graph, CoreProviders context) {
         canonicalizer.applyIncremental(graph, context, changedNodes.getNodes());
     }
 
-    //@formatter:off
     /**
-     * Re-associate loop invariant so that invariant parts of the expression can move outside of the
-     * loop.
+     * Re-associate loop invariant so that invariant parts of the expression can move out of the
+     * loop. For example:
      *
-     * For example:
-     *     for (int i = 0; i < LENGTH; i++) {         for (int i = 0; i < LENGTH; i++) {
-     *         arr[i] = (i * inv1) * inv2;       =>       arr[i] = i * (inv1 * inv2);
-     *     }                                          }
+     * <pre>
+     * for (int i = 0; i < LENGTH; i++) {
+     *     arr[i] = (i * inv1) * inv2;
+     * }
+     * </pre>
+     *
+     * is transformed to
+     *
+     * <pre>
+     *
+     * for (int i = 0; i < LENGTH; i++) {
+     *     arr[i] = i * (inv1 * inv2);
+     * }
+     * </pre>
      */
-    //@formatter:on
     @SuppressWarnings("try")
     private static void reassociateInvariants(StructuredGraph graph, CoreProviders context) {
         DebugContext debug = graph.getDebug();
@@ -101,7 +109,11 @@ private static void reassociateInvariants(StructuredGraph graph, CoreProviders c
             // bound.
             while (changed && iterations < 32) {
                 changed = false;
-                for (Loop loop : loopsData.loops()) {
+                /*
+                 * Process inner loops last to ensure loop phis are used as late as possible. This
+                 * minimizes dependency chains in the backedge value calculation on register level.
+                 */
+                for (Loop loop : loopsData.outerFirst()) {
                     changed |= loop.reassociateInvariants();
                 }
                 loopsData.deleteUnusedNodes();