[GR-59627] Aggressively hoist trusted final fields

PullRequest: graal/20821

Author: kittyoracle

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

@@ -25,6 +25,8 @@
 package jdk.graal.compiler.truffle.test;
 
 import java.io.IOException;
+import java.lang.invoke.VarHandle;
+import java.util.Objects;
 import java.util.function.Consumer;
 
 import org.graalvm.word.LocationIdentity;
@@ -33,11 +35,14 @@
 
 import com.oracle.truffle.api.Truffle;
 import com.oracle.truffle.api.frame.FrameDescriptor;
+import com.oracle.truffle.api.frame.FrameSlotKind;
 import com.oracle.truffle.api.impl.FrameWithoutBoxing;
 import com.oracle.truffle.api.test.SubprocessTestUtils;
 
 import jdk.graal.compiler.api.directives.GraalDirectives;
+import jdk.graal.compiler.debug.GraalError;
 import jdk.graal.compiler.graph.Node;
+import jdk.graal.compiler.nodeinfo.Verbosity;
 import jdk.graal.compiler.nodes.FieldLocationIdentity;
 import jdk.graal.compiler.nodes.NamedLocationIdentity;
 import jdk.graal.compiler.nodes.StructuredGraph;
@@ -82,6 +87,9 @@ public void testStaticReads() throws IOException, InterruptedException {
                     LocationIdentity identity = read.getLocationIdentity();
                     if (identity instanceof FieldLocationIdentity) {
                         fieldReads++;
+                        if (fieldReads > 1) {
+                            GraalError.guarantee(false, "%s", read.toString(Verbosity.All));
+                        }
                     } else if (NamedLocationIdentity.isArrayLocation(identity)) {
                         arrayReads++;
                     } else if (identity == NamedLocationIdentity.ARRAY_LENGTH_LOCATION) {
@@ -141,6 +149,34 @@ public void testReadsWritesWithoutZeroExtend() throws IOException, InterruptedEx
         }).disableAssertions(FrameWithoutBoxing.class).postfixVmOption("-Djdk.graal.TruffleTrustedFinalFrameFields=true").run();
     }
 
+    static FrameWithoutBoxing escape;
+    static final Object[] EMPTY_ARRAY = new Object[0];
+
+    /**
+     * Test that the loads should not be hoisted incorrectly.
+     */
+    public static Object snippetNoMoveReads(FrameDescriptor desc, Object obj) {
+        FrameWithoutBoxing frame = new FrameWithoutBoxing(desc, EMPTY_ARRAY);
+        frame.setObject(0, obj);
+        VarHandle.fullFence();
+        // Don't let the frame be scalar replaced
+        escape = frame;
+        VarHandle.fullFence();
+        return frame.getObject(0);
+    }
+
+    @Test
+    public void testNoMoveReads() throws IOException, InterruptedException {
+        SubprocessTestUtils.newBuilder(FrameHostReadsTest.class, () -> {
+            FrameDescriptor.Builder descBuilder = FrameDescriptor.newBuilder();
+            descBuilder.addSlot(FrameSlotKind.Object, null, null);
+            FrameDescriptor desc = descBuilder.build();
+            Object obj = new Object();
+            Result res = test("snippetNoMoveReads", desc, obj);
+            Assert.assertSame(Objects.toString(res.returnValue), obj, res.returnValue);
+        }).disableAssertions(FrameWithoutBoxing.class).postfixVmOption("-Djdk.graal.TruffleTrustedFinalFrameFields=true").run();
+    }
+
     private void compileAndCheck(String snippet, Consumer<StructuredGraph> check) {
         getTruffleCompiler();
         initAssertionError();

compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/core/common/spi/ConstantFieldProvider.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2016, 2018, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2016, 2025, 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
@@ -24,8 +24,14 @@
  */
 package jdk.graal.compiler.core.common.spi;
 
-import jdk.graal.compiler.options.OptionValues;
+import java.lang.foreign.MemorySegment;
 
+import jdk.graal.compiler.core.common.util.PhasePlan;
+import jdk.graal.compiler.nodes.Invoke;
+import jdk.graal.compiler.nodes.StartNode;
+import jdk.graal.compiler.nodes.memory.FloatingReadNode;
+import jdk.graal.compiler.nodes.spi.CanonicalizerTool;
+import jdk.graal.compiler.options.OptionValues;
 import jdk.vm.ci.meta.JavaConstant;
 import jdk.vm.ci.meta.ResolvedJavaField;
 
@@ -68,4 +74,70 @@ interface ConstantFieldTool<T> {
     default boolean maybeFinal(@SuppressWarnings("unused") ResolvedJavaField field) {
         return false;
     }
+
+    /**
+     * Returns {@code true} if {@code field} is final and considered "trusted". A trusted final
+     * field is immutable after the initialization of the holder object. This allows us to schedule
+     * a load from {@code field} freely up until the point at which we can guarantee that the object
+     * is initialized. It also allows us to assume that there is no store to the field after it is
+     * read, skipping the anti-dependency calculation while scheduling.
+     * <p>
+     * Example 1:
+     * {@snippet :
+     * class Holder {
+     *     final int x;
+     * }
+     *
+     * void test(Holder h) {
+     *     for (int i = 0; i < 100; i++) {
+     *         nonInlinedCall(h.x);
+     *     }
+     * }
+     * }
+     * Without further contexts, we cannot assume that {@code nonInlineCall} will not modify
+     * {@code h.x}, even if it is a final field, because {@code Unsafe} and the core reflection API
+     * can be used to modify a final field even after initialization. Additionally, there's a risk
+     * that a constructor of {@code Holder} may use {@code Holder.x} as a flag for inter-thread
+     * communication. As a result, the load from {@code h.x} needs to be executed in each iteration.
+     * However, if we can trust that {@code h} should be initialized before being passed to
+     * {@code test} and {@code x} should not be modified post-initialization, then we can hoist the
+     * load outside the loop.
+     * <p>
+     * When a {@link FloatingReadNode} reads from a field for which this method returns
+     * {@code true}, we rewire its memory input to the earliest location possible in the CFG. In the
+     * example above, we rewire the memory input of the load {@code h.x} to the {@link StartNode} of
+     * the method {@code test}. This allows the load to have the maximum freedom in scheduling, as
+     * well as enables all the loads from {@code h.x} to be GVN-ed by making their memory inputs
+     * identical. Note that the load is still not immutable, this is important because if the method
+     * {@code test} is later inlined into another graph, it may turn out that {@code h} is created
+     * and {@code h.x} is assigned in the caller. In those cases, marking the load node as immutable
+     * would be incorrect. On the other hand, since the memory input of the node is the
+     * {@link StartNode} of the method {@code test}, after inlining, this node is expanded into the
+     * memory node corresponding to the input memory state of the invocation to {@code test} in the
+     * caller graph, we can still ensure that the load is not scheduled too freely which can bypass
+     * the initialization in the caller. This is generally not a concern because
+     * {@link FloatingReadNode}s are only introduced after the high tier, while inlining is only
+     * performed during high tier. However, there may be other mechanisms such as snippets which can
+     * inline code in a limited manner after high tier, and it is more future-proof to not rely on
+     * the {@link PhasePlan} for the correctness of this transformation.
+     * <p>
+     * To ensure that the load has the maximum freedom in scheduling, we mark that the load has no
+     * anti-dependency. In other word, it aliases with no store that is dominated by its memory
+     * input, and we do not have to take anti-dependency into consideration while scheduling the
+     * load. This is necessary both in terms of correctness and efficiency. As moving the memory
+     * input may introduce incorrect anti-dependency with nodes between the old and the new memory
+     * input in the CFG, potentially leading to an unschedulable graph.
+     * <p>
+     * This transformation can be performed if the holder object is:
+     * <ul>
+     * <li>A parameter that is not the receiver of a constructor, the earliest memory input can be
+     * the {@link StartNode} of the graph.
+     * <li>A value returned from a method invocation, the earliest memory input can be the
+     * {@link Invoke} corresponding to the invocation (to do).
+     * </ul>
+     * It can be trivially extended to other trusted final fields in the JDK (e.g.
+     * {@link MemorySegment}). However, extending it to general uses would risk violating the JVM
+     * semantics.
+     */
+    boolean isTrustedFinal(CanonicalizerTool tool, ResolvedJavaField field);
 }

compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/core/common/spi/JavaConstantFieldProvider.java

@@ -27,6 +27,7 @@
 import java.util.Arrays;
 
 import jdk.graal.compiler.debug.GraalError;
+import jdk.graal.compiler.nodes.spi.CanonicalizerTool;
 import jdk.graal.compiler.options.Option;
 import jdk.graal.compiler.options.OptionKey;
 import jdk.vm.ci.meta.JavaConstant;
@@ -197,4 +198,9 @@ protected boolean isWellKnownImplicitStableField(ResolvedJavaField field) {
         }
         return field.equals(stringValueField);
     }
+
+    @Override
+    public boolean isTrustedFinal(CanonicalizerTool tool, ResolvedJavaField field) {
+        return false;
+    }
 }

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

@@ -77,21 +77,29 @@ public final class LoadFieldNode extends AccessFieldNode implements Canonicaliza
 
     private final Stamp uncheckedStamp;
 
+    /**
+     * Records the injection of the property that this is a load from a trusted final field.
+     *
+     * @see ConstantFieldProvider#isTrustedFinal(CanonicalizerTool, ResolvedJavaField)
+     */
+    private final boolean trustInjected;
+
     protected LoadFieldNode(StampPair stamp, ValueNode object, ResolvedJavaField field, boolean immutable) {
-        this(stamp, object, field, MemoryOrderMode.getMemoryOrder(field), immutable);
+        this(stamp, object, field, MemoryOrderMode.getMemoryOrder(field), immutable, false);
     }
 
-    protected LoadFieldNode(StampPair stamp, ValueNode object, ResolvedJavaField field, MemoryOrderMode memoryOrder, boolean immutable) {
+    protected LoadFieldNode(StampPair stamp, ValueNode object, ResolvedJavaField field, MemoryOrderMode memoryOrder, boolean immutable, boolean trustInjected) {
         super(TYPE, stamp.getTrustedStamp(), object, field, memoryOrder, immutable);
         this.uncheckedStamp = stamp.getUncheckedStamp();
+        this.trustInjected = trustInjected;
     }
 
     public static LoadFieldNode create(Assumptions assumptions, ValueNode object, ResolvedJavaField field) {
         return create(assumptions, object, field, MemoryOrderMode.getMemoryOrder(field));
     }
 
     public static LoadFieldNode create(Assumptions assumptions, ValueNode object, ResolvedJavaField field, MemoryOrderMode memoryOrder) {
-        return new LoadFieldNode(StampFactory.forDeclaredType(assumptions, field.getType(), false), object, field, memoryOrder, false);
+        return new LoadFieldNode(StampFactory.forDeclaredType(assumptions, field.getType(), false), object, field, memoryOrder, false, false);
     }
 
     public static ValueNode create(ConstantFieldProvider constantFields, ConstantReflectionProvider constantReflection, MetaAccessProvider metaAccess,
@@ -113,6 +121,20 @@ public static ValueNode createOverrideStamp(ConstantFieldProvider constantFields
         return canonical(null, stamp, object, field, constantFields, constantReflection, options, metaAccess, canonicalizeReads, allUsagesAvailable, false, position);
     }
 
+    /**
+     * Returns a node that is the same as this node but is injected with the property that it is a
+     * load from a trusted final field. May return this node if the injection is unnecessary or has
+     * already been done.
+     *
+     * @see ConstantFieldProvider#isTrustedFinal(CanonicalizerTool, ResolvedJavaField)
+     */
+    public LoadFieldNode withTrustInjected() {
+        if (getLocationIdentity().isImmutable() || trustInjected) {
+            return this;
+        }
+        return new LoadFieldNode(StampPair.create(stamp, uncheckedStamp), object(), field(), getMemoryOrder(), false, true);
+    }
+
     @Override
     public LocationIdentity getKilledLocationIdentity() {
         if (ordersMemoryAccesses()) {
@@ -237,4 +259,14 @@ public NodeCycles estimatedNodeCycles() {
         }
         return super.estimatedNodeCycles();
     }
+
+    /**
+     * Returns {@code true} if this node is injected with the property that it is a load from a
+     * trusted final field.
+     *
+     * @see ConstantFieldProvider#isTrustedFinal(CanonicalizerTool, ResolvedJavaField)
+     */
+    public boolean trustInjected() {
+        return trustInjected;
+    }
 }