[GR-51560] Simplify rbp handling in prologues/epilogues.

substratevm/src/com.oracle.svm.core.graal.amd64/src/com/oracle/svm/core/graal/amd64/AMD64CalleeSavedRegisters.java

@@ -101,6 +101,12 @@ public static void createAndRegister() {
             calleeSavedRegisters.remove(SubstrateControlFlowIntegrity.singleton().getCFITargetRegister());
         }
 
+        /*
+         * Handling of the rbp register is always done in the method prologue, so we remove it here
+         * and record whether it is callee saved.
+         */
+        boolean rbpCalleeSaved = calleeSavedRegisters.remove(AMD64.rbp);
+
         /*
          * Reverse list so that CPU registers are spilled close to the beginning of the frame, i.e.,
          * with a closer-to-0 negative reference map index in the caller frame. That makes the
@@ -151,9 +157,18 @@ public static void createAndRegister() {
         int calleeSavedRegistersSizeInBytes = offset;
 
         int saveAreaOffsetInFrame = -(FrameAccess.returnAddressSize() +
-                        (SubstrateOptions.PreserveFramePointer.getValue() ? FrameAccess.wordSize() : 0) +
+                        FrameAccess.wordSize() + /* Space is always reserved for rbp. */
                         calleeSavedRegistersSizeInBytes);
 
+        if (rbpCalleeSaved) {
+            /*
+             * When rbp is callee saved, it is pushed onto the stack in the method prologue right
+             * after the return address, i.e., at the top of the callee save area, so we are just
+             * referencing that location here.
+             */
+            calleeSavedRegisterOffsets.put(AMD64.rbp, calleeSavedRegistersSizeInBytes);
+        }
+
         ImageSingletons.add(CalleeSavedRegisters.class,
                         new AMD64CalleeSavedRegisters(frameRegister, calleeSavedRegisters, calleeSavedXMMRegisters, calleeSavedMaskRegisters, calleeSavedRegisterOffsets,
                                         calleeSavedRegistersSizeInBytes, saveAreaOffsetInFrame, isRuntimeCompilationEnabled));
@@ -537,7 +552,7 @@ private static void dumpReg(Log log, String label, Pointer callerSP, int offsetI
     @Fold
     static int offsetInFrameOrNull(Register register) {
         AMD64CalleeSavedRegisters that = AMD64CalleeSavedRegisters.singleton();
-        if (that.calleeSavedRegisters.contains(register)) {
+        if (that.calleeSaveable(register)) {
             return that.getOffsetInFrame(register);
         } else {
             return 0;

substratevm/src/com.oracle.svm.core.graal.amd64/src/com/oracle/svm/core/graal/amd64/AMD64FarReturnOp.java

@@ -97,7 +97,7 @@ public void emitCode(CompilationResultBuilder crb, AMD64MacroAssembler masm) {
          * of the new stack pointer.
          */
         int calleeFrameSize = FrameAccess.returnAddressSize();
-        if (SubstrateOptions.PreserveFramePointer.getValue()) {
+        if (fromMethodWithCalleeSavedRegisters || SubstrateOptions.PreserveFramePointer.getValue()) {
             calleeFrameSize += FrameAccess.wordSize();
         }
         if (fromMethodWithCalleeSavedRegisters) {
@@ -117,7 +117,7 @@ public void emitCode(CompilationResultBuilder crb, AMD64MacroAssembler masm) {
             AMD64CalleeSavedRegisters.singleton().emitRestore(masm, calleeFrameSize, asRegister(result), crb);
             masm.incrementq(AMD64.rsp, CalleeSavedRegisters.singleton().getSaveAreaSize());
         }
-        if (SubstrateOptions.PreserveFramePointer.getValue()) {
+        if (fromMethodWithCalleeSavedRegisters || SubstrateOptions.PreserveFramePointer.getValue()) {
             masm.pop(AMD64.rbp);
         }
 

substratevm/src/com.oracle.svm.core.graal.amd64/src/com/oracle/svm/core/graal/amd64/SubstrateAMD64Backend.java

@@ -188,6 +188,7 @@
 import jdk.vm.ci.code.CompilationRequest;
 import jdk.vm.ci.code.CompiledCode;
 import jdk.vm.ci.code.Register;
+import jdk.vm.ci.code.RegisterAttributes;
 import jdk.vm.ci.code.RegisterConfig;
 import jdk.vm.ci.code.RegisterValue;
 import jdk.vm.ci.code.StackSlot;
@@ -1097,6 +1098,41 @@ private static ForeignCallDescriptor chooseCPUFeatureVariant(ForeignCallDescript
         }
     }
 
+    /**
+     * Generates a method's prologue and epilogue.
+     * <p>
+     * Depending on the {@link SubstrateAMD64FrameMap frame map} properties and whether the rbp
+     * register is saved by the caller or the callee, we use different forms of prologue and
+     * epilogue.
+     *
+     * <pre>
+     *
+     *          |      preserveFramePointer       |
+     *          +----------------+----------------+
+     *          |     false      |      true      |
+     *  --------+----------------+----------------+
+     *          |  ; prologue    |  ; prologue    |
+     *          |  sub rsp, #fs  |  push rbp      |
+     *          |                |  mov rbp, rsp  |
+     *  rbp is  |                |  sub rsp, #fs  |
+     *  caller  |                |                |
+     *  saved   |  ; epilogue    |  ; epilogue    |
+     *          |  add rsp, #fs  |  add rsp, #fs  |
+     *          |  ret           |  pop rbp       |
+     *          |                |  ret           |
+     *  --------+----------------+----------------+
+     *          |  ; prologue    |  ; prologue    |
+     *          |  push rbp      |  push rbp      |
+     *          |  sub rsp, #fs  |  mov rbp, rsp  |
+     *  rbp is  |                |  sub rsp, #fs  |
+     *  callee  |                |                |
+     *  saved   |  ; epilogue    |  ; epilogue    |
+     *          |  add rsp, #fs  |  add rsp, #fs  |
+     *          |  pop rbp       |  pop rbp       |
+     *          |  ret           |  ret           |
+     *  --------+----------------+----------------+
+     * </pre>
+     */
     protected static class SubstrateAMD64FrameContext implements FrameContext {
 
         protected final SharedMethod method;
@@ -1132,36 +1168,38 @@ protected final void reserveStackFrame(CompilationResultBuilder crb, AMD64MacroA
         }
 
         protected void maybePushBasePointer(CompilationResultBuilder crb, AMD64MacroAssembler asm) {
-            if (((SubstrateAMD64RegisterConfig) crb.frameMap.getRegisterConfig()).shouldUseBasePointer()) {
+            SubstrateAMD64FrameMap frameMap = (SubstrateAMD64FrameMap) crb.frameMap;
+            if (frameMap.preserveFramePointer()) {
                 /*
                  * Note that we never use the `enter` instruction so that we have a predictable code
                  * pattern at each method prologue. And `enter` seems to be slower than the explicit
                  * code.
                  */
                 asm.push(rbp);
                 asm.movq(rbp, rsp);
+            } else if (isCalleeSaved(rbp, frameMap.getRegisterConfig(), method)) {
+                asm.push(rbp);
             }
         }
 
         @Override
         public void leave(CompilationResultBuilder crb) {
             AMD64MacroAssembler asm = (AMD64MacroAssembler) crb.asm;
+            SubstrateAMD64FrameMap frameMap = (SubstrateAMD64FrameMap) crb.frameMap;
             crb.recordMark(SubstrateMarkId.EPILOGUE_START);
 
             if (method.hasCalleeSavedRegisters()) {
                 JavaKind returnKind = method.getSignature().getReturnKind();
                 Register returnRegister = null;
                 if (returnKind != JavaKind.Void) {
-                    returnRegister = crb.frameMap.getRegisterConfig().getReturnRegister(returnKind);
+                    returnRegister = frameMap.getRegisterConfig().getReturnRegister(returnKind);
                 }
-                AMD64CalleeSavedRegisters.singleton().emitRestore((AMD64MacroAssembler) crb.asm, crb.frameMap.totalFrameSize(), returnRegister, crb);
+                AMD64CalleeSavedRegisters.singleton().emitRestore(asm, frameMap.totalFrameSize(), returnRegister, crb);
             }
 
-            if (((SubstrateAMD64RegisterConfig) crb.frameMap.getRegisterConfig()).shouldUseBasePointer()) {
-                asm.movq(rsp, rbp);
+            asm.incrementq(rsp, frameMap.frameSize());
+            if (frameMap.preserveFramePointer() || isCalleeSaved(rbp, frameMap.getRegisterConfig(), method)) {
                 asm.pop(rbp);
-            } else {
-                asm.incrementq(rsp, crb.frameMap.frameSize());
             }
 
             crb.recordMark(SubstrateMarkId.EPILOGUE_INCD_RSP);
@@ -1412,11 +1450,31 @@ public void emitCode(CompilationResultBuilder crb, AMD64MacroAssembler masm) {
         }
     }
 
-    private FrameMapBuilder newFrameMapBuilder(RegisterConfig registerConfig) {
-        RegisterConfig registerConfigNonNull = registerConfig == null ? getCodeCache().getRegisterConfig() : registerConfig;
-        FrameMap frameMap = new AMD64FrameMap(getProviders().getCodeCache(), registerConfigNonNull, new SubstrateReferenceMapBuilderFactory(),
-                        ((SubstrateAMD64RegisterConfig) registerConfigNonNull).shouldUseBasePointer());
-        return new AMD64FrameMapBuilder(frameMap, getCodeCache(), registerConfigNonNull);
+    private FrameMapBuilder newFrameMapBuilder(RegisterConfig registerConfig, SharedMethod method) {
+        FrameMap frameMap = new SubstrateAMD64FrameMap(getCodeCache(), (SubstrateAMD64RegisterConfig) registerConfig, new SubstrateReferenceMapBuilderFactory(), method);
+        return new AMD64FrameMapBuilder(frameMap, getCodeCache(), registerConfig);
+    }
+
+    /**
+     * AMD64 Substrate VM specific frame map.
+     * <p>
+     * The layout is basically the same as {@link AMD64FrameMap} except that space for rbp is also
+     * reserved when rbp is callee saved, not just if {@link #preserveFramePointer} is true.
+     */
+    static class SubstrateAMD64FrameMap extends AMD64FrameMap {
+        SubstrateAMD64FrameMap(CodeCacheProvider codeCache, SubstrateAMD64RegisterConfig registerConfig, ReferenceMapBuilderFactory referenceMapFactory, SharedMethod method) {
+            super(codeCache, registerConfig, referenceMapFactory, registerConfig.shouldUseBasePointer());
+            if (!preserveFramePointer() && isCalleeSaved(rbp, registerConfig, method)) {
+                assert initialSpillSize == returnAddressSize() && spillSize == initialSpillSize : "rbp must be right after the return address";
+                initialSpillSize += getTarget().wordSize;
+                spillSize += getTarget().wordSize;
+            }
+        }
+    }
+
+    private static boolean isCalleeSaved(Register register, RegisterConfig config, SharedMethod method) {
+        RegisterAttributes registerAttributes = config.getAttributesMap()[register.number];
+        return registerAttributes.isCalleeSave() || registerAttributes.isAllocatable() && method.hasCalleeSavedRegisters();
     }
 
     @Override
@@ -1425,7 +1483,7 @@ public LIRGenerationResult newLIRGenerationResult(CompilationIdentifier compilat
         SubstrateCallingConventionKind ccKind = method.getCallingConventionKind();
         SubstrateCallingConventionType ccType = ccKind.isCustom() ? method.getCustomCallingConventionType() : ccKind.toType(false);
         CallingConvention callingConvention = CodeUtil.getCallingConvention(getCodeCache(), ccType, method, this);
-        return new SubstrateLIRGenerationResult(compilationId, lir, newFrameMapBuilder(registerAllocationConfig.getRegisterConfig()), callingConvention, registerAllocationConfig, method);
+        return new SubstrateLIRGenerationResult(compilationId, lir, newFrameMapBuilder(registerAllocationConfig.getRegisterConfig(), method), callingConvention, registerAllocationConfig, method);
     }
 
     protected AMD64ArithmeticLIRGenerator createArithmeticLIRGen(RegisterValue nullRegisterValue) {

substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/FrameAccess.java

@@ -68,13 +68,6 @@ public static int returnAddressSize() {
     @Fold
     public abstract int stackPointerAdjustmentOnCall();
 
-    /**
-     * Returns the size in bytes of the saved base pointer in the stack frame. The saved base
-     * pointer must be located immediately after the return address (if this is not the case in a
-     * new architecture, bigger modifications to code like the Deoptimizer is required).
-     */
-    public abstract int savedBasePointerSize();
-
     @Fold
     public static int wordSize() {
         return ConfigurationValues.getTarget().arch.getWordSize();

substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/aarch64/AArch64FrameAccess.java

@@ -24,7 +24,6 @@
  */
 package com.oracle.svm.core.aarch64;
 
-import jdk.graal.compiler.api.replacements.Fold;
 import org.graalvm.nativeimage.Platform;
 import org.graalvm.nativeimage.Platforms;
 import org.graalvm.nativeimage.c.function.CodePointer;
@@ -36,6 +35,8 @@
 import com.oracle.svm.core.feature.AutomaticallyRegisteredImageSingleton;
 import com.oracle.svm.core.graal.nodes.aarch64.AArch64XPACNode;
 
+import jdk.graal.compiler.api.replacements.Fold;
+
 @AutomaticallyRegisteredImageSingleton(FrameAccess.class)
 @Platforms(Platform.AARCH64.class)
 public class AArch64FrameAccess extends FrameAccess {
@@ -63,13 +64,6 @@ public Pointer getReturnAddressLocation(Pointer sourceSp) {
         return sourceSp.subtract(returnAddressSize());
     }
 
-    @Fold
-    @Override
-    public int savedBasePointerSize() {
-        // The base pointer is always saved with stp instruction on method entry
-        return wordSize();
-    }
-
     @Override
     @Fold
     public int stackPointerAdjustmentOnCall() {

substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/amd64/AMD64FrameAccess.java

@@ -24,17 +24,17 @@
  */
 package com.oracle.svm.core.amd64;
 
-import jdk.graal.compiler.api.replacements.Fold;
 import org.graalvm.nativeimage.Platform.AMD64;
 import org.graalvm.nativeimage.Platforms;
 import org.graalvm.nativeimage.c.function.CodePointer;
 import org.graalvm.word.Pointer;
 
 import com.oracle.svm.core.FrameAccess;
-import com.oracle.svm.core.SubstrateOptions;
 import com.oracle.svm.core.Uninterruptible;
 import com.oracle.svm.core.feature.AutomaticallyRegisteredImageSingleton;
 
+import jdk.graal.compiler.api.replacements.Fold;
+
 @AutomaticallyRegisteredImageSingleton(FrameAccess.class)
 @Platforms(AMD64.class)
 public final class AMD64FrameAccess extends FrameAccess {
@@ -58,16 +58,6 @@ public Pointer getReturnAddressLocation(Pointer sourceSp) {
         return sourceSp.subtract(returnAddressSize());
     }
 
-    @Fold
-    @Override
-    public int savedBasePointerSize() {
-        if (SubstrateOptions.PreserveFramePointer.getValue()) {
-            return wordSize();
-        } else {
-            return 0;
-        }
-    }
-
     @Override
     @Fold
     public int stackPointerAdjustmentOnCall() {

substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/deopt/Deoptimizer.java

@@ -33,7 +33,9 @@
 import java.util.ArrayList;
 
 import org.graalvm.nativeimage.CurrentIsolate;
+import org.graalvm.nativeimage.ImageSingletons;
 import org.graalvm.nativeimage.IsolateThread;
+import org.graalvm.nativeimage.Platform;
 import org.graalvm.nativeimage.c.function.CodePointer;
 import org.graalvm.word.Pointer;
 import org.graalvm.word.PointerBase;
@@ -82,6 +84,7 @@
 import com.oracle.svm.core.util.TimeUtils;
 import com.oracle.svm.core.util.VMError;
 
+import jdk.graal.compiler.api.replacements.Fold;
 import jdk.graal.compiler.core.common.NumUtil;
 import jdk.graal.compiler.core.common.util.TypeConversion;
 import jdk.graal.compiler.options.Option;
@@ -611,7 +614,7 @@ private static UnsignedWord rewriteStackStub(Pointer newSp, UnsignedWord gpRetur
          * deoptimizeInRange(). So when this method returns we are inside the caller of
          * deoptimizeInRange().
          */
-        Pointer bottomSp = newSp.subtract(FrameAccess.returnAddressSize() + FrameAccess.singleton().savedBasePointerSize());
+        Pointer bottomSp = newSp.subtract(FrameAccess.returnAddressSize() + savedBasePointerSize());
         frame.getTargetContent().copyToPointer(bottomSp);
 
         if (DeoptimizationCounters.Options.ProfileDeoptimization.getValue()) {
@@ -621,6 +624,25 @@ private static UnsignedWord rewriteStackStub(Pointer newSp, UnsignedWord gpRetur
         return gpReturnValue;
     }
 
+    /**
+     * Returns the size in bytes of the saved base pointer in the stack frame. The saved base
+     * pointer must be located immediately after the return address (if this is not the case in a
+     * new architecture, bigger modifications to the Deoptimizer code are required).
+     */
+    @Fold
+    static int savedBasePointerSize() {
+        if (Platform.includedIn(Platform.AMD64.class)) {
+            return SubstrateOptions.PreserveFramePointer.getValue() ? FrameAccess.wordSize() : 0;
+        } else if (Platform.includedIn(Platform.AARCH64.class)) {
+            // The base pointer is always saved with stp instruction on method entry
+            return FrameAccess.wordSize();
+        } else if (Platform.includedIn(Platform.RISCV64.class)) {
+            // The base pointer is always pushed on the stack on method entry
+            return FrameAccess.wordSize();
+        }
+        throw VMError.shouldNotReachHere("Unexpected platform: " + ImageSingletons.lookup(Platform.class));
+    }
+
     /**
      * Reads the value of a local variable in the given frame. If the local variable is a virtual
      * object, the object (and all other objects reachable from it) are materialized.
@@ -799,7 +821,7 @@ public static void logRecentDeoptimizationEvents(Log log) {
      */
     private VirtualFrame constructTargetFrame(CodeInfoQueryResult targetInfo, FrameInfoQueryResult sourceFrame) {
         FrameInfoQueryResult targetFrame = targetInfo.getFrameInfo();
-        int savedBasePointerSize = FrameAccess.singleton().savedBasePointerSize();
+        int savedBasePointerSize = savedBasePointerSize();
         int targetFrameSize = NumUtil.safeToInt(targetInfo.getTotalFrameSize()) - FrameAccess.returnAddressSize() - savedBasePointerSize;
         VirtualFrame result = new VirtualFrame(targetFrame);
 

substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/jfr/sampler/AbstractJfrExecutionSampler.java

@@ -332,7 +332,7 @@ private static void doUninterruptibleStackWalk(SamplerSampleWriterData data, Poi
                  * We are in the prologue or epilogue. Frame pointer and return address are on top
                  * of the stack.
                  */
-                callerSP = sp.add(FrameAccess.wordSize()).add(FrameAccess.singleton().savedBasePointerSize());
+                callerSP = sp.add(FrameAccess.wordSize()).add(FrameAccess.wordSize());
             }
         } else {
             /* We are in the prologue or epilogue. Return address is at the top of the stack. */

substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/riscv64/RISCV64FrameAccess.java

@@ -24,7 +24,6 @@
  */
 package com.oracle.svm.core.riscv64;
 
-import jdk.graal.compiler.api.replacements.Fold;
 import org.graalvm.nativeimage.Platform;
 import org.graalvm.nativeimage.Platforms;
 import org.graalvm.nativeimage.c.function.CodePointer;
@@ -34,6 +33,8 @@
 import com.oracle.svm.core.Uninterruptible;
 import com.oracle.svm.core.feature.AutomaticallyRegisteredImageSingleton;
 
+import jdk.graal.compiler.api.replacements.Fold;
+
 @AutomaticallyRegisteredImageSingleton(FrameAccess.class)
 @Platforms(Platform.RISCV64.class)
 public class RISCV64FrameAccess extends FrameAccess {
@@ -56,13 +57,6 @@ public Pointer getReturnAddressLocation(Pointer sourceSp) {
         return sourceSp.subtract(returnAddressSize());
     }
 
-    @Fold
-    @Override
-    public int savedBasePointerSize() {
-        // The base pointer is always pushed on the stack on method entry
-        return wordSize();
-    }
-
     @Override
     @Fold
     public int stackPointerAdjustmentOnCall() {