[JDK-8068945] Use RBP register as proper frame pointer in JIT compiled code on x86 - Java Bug System

Details

Type: Enhancement
Status: Resolved
Priority: P4
Resolution: Fixed
Affects Version/s: 9
Fix Version/s: 9
Component/s: hotspot
Labels:
None

Subcomponent:
compiler
Resolved In Build:
b64
CPU:

x86_64
OS:

generic

Backports

Issue	Fix Version	Assignee	Priority	Status	Resolution	Resolved In Build
JDK-8072465	8u60	Zoltan Majo	P4	Resolved	Fixed	b19
JDK-8082711	emb-9	Zoltan Majo	P4	Resolved	Fixed	team
JDK-8087250	8u65	Zoltan Majo	P4	Resolved	Fixed	b01
JDK-8129650	emb-8u60	Zoltan Majo	P4	Resolved	Fixed	b19
JDK-8137682	emb-8u65	Unassigned	P4	Resolved	Fixed	b01

Description

We used rbp register for local values in jit compiled code on x86 because 32-bit mode have only few registers.
We do the same in 64-bit VM but it may help less with performance since it has more registers.
Using RBP only as frame pointer will help debugging and profiling tools.

Issue Links

relates to

JDK-8080650 Enable stubs to use frame pointers correctly

JDK-8080281 8068945 changes break building the zero JVM variant

JDK-8076227 Cleanup unused method invocation logic

JDK-8153167 Allow stack walking pass through method handle intrinsic frames

JDK-8075798 Allow ADLC register class to depend on runtime conditions also for cisc-spillable classes

Options
- Show All
- Show Open

Sub-Tasks

1.

Add documentation for JDK-8068945 to Tools Reference Guide

In Progress

Shilpa Chetan

2016-12-16

Activity

Ascending order - Click to sort in descending order

Hide

Permalink

Vladimir Kozlov added a comment - 2015-01-13 18:00

Recently we received request with proposed patch from Brendan Gregg:

In case there's problems with the patch URL, the patch is:

--- openjdk8clean/hotspot/src/cpu/x86/vm/x86_64.ad 2014-03-04 02:52:11.000000000 +0000
+++ openjdk8/hotspot/src/cpu/x86/vm/x86_64.ad 2014-11-08 01:10:49.686044933 +0000
@@ -166,10 +166,9 @@
// 3) reg_class stack_slots( /* one chunk of stack-based "registers" */ )
//

-// Class for all pointer registers (including RSP)
+// Class for all pointer registers (including RSP, excluding RBP)
reg_class any_reg(RAX, RAX_H,
                   RDX, RDX_H,
- RBP, RBP_H,
                   RDI, RDI_H,
                   RSI, RSI_H,
                   RCX, RCX_H,
@@ -184,10 +183,9 @@
                   R14, R14_H,
                   R15, R15_H);

-// Class for all pointer registers except RSP
+// Class for all pointer registers except RSP and RBP
reg_class ptr_reg(RAX, RAX_H,
                   RDX, RDX_H,
- RBP, RBP_H,
                   RDI, RDI_H,
                   RSI, RSI_H,
                   RCX, RCX_H,
@@ -199,9 +197,8 @@
                   R13, R13_H,
                   R14, R14_H);

-// Class for all pointer registers except RAX and RSP
+// Class for all pointer registers except RAX, RSP and RBP
reg_class ptr_no_rax_reg(RDX, RDX_H,
- RBP, RBP_H,
                          RDI, RDI_H,
                          RSI, RSI_H,
                          RCX, RCX_H,
@@ -226,9 +223,8 @@
                          R13, R13_H,
                          R14, R14_H);

-// Class for all pointer registers except RAX, RBX and RSP
+// Class for all pointer registers except RAX, RBX, RSP and RBP
reg_class ptr_no_rax_rbx_reg(RDX, RDX_H,
- RBP, RBP_H,
                              RDI, RDI_H,
                              RSI, RSI_H,
                              RCX, RCX_H,
@@ -260,10 +256,9 @@
// Singleton class for TLS pointer
reg_class ptr_r15_reg(R15, R15_H);

-// Class for all long registers (except RSP)
+// Class for all long registers (except RSP and RBP)
reg_class long_reg(RAX, RAX_H,
                    RDX, RDX_H,
- RBP, RBP_H,
                    RDI, RDI_H,
                    RSI, RSI_H,
                    RCX, RCX_H,
@@ -275,9 +270,8 @@
                    R13, R13_H,
                    R14, R14_H);

-// Class for all long registers except RAX, RDX (and RSP)
-reg_class long_no_rax_rdx_reg(RBP, RBP_H,
- RDI, RDI_H,
+// Class for all long registers except RAX, RDX (and RSP, RBP)
+reg_class long_no_rax_rdx_reg(RDI, RDI_H,
                               RSI, RSI_H,
                               RCX, RCX_H,
                               RBX, RBX_H,
@@ -288,9 +282,8 @@
                               R13, R13_H,
                               R14, R14_H);

-// Class for all long registers except RCX (and RSP)
-reg_class long_no_rcx_reg(RBP, RBP_H,
- RDI, RDI_H,
+// Class for all long registers except RCX (and RSP, RBP)
+reg_class long_no_rcx_reg(RDI, RDI_H,
                           RSI, RSI_H,
                           RAX, RAX_H,
                           RDX, RDX_H,
@@ -302,9 +295,8 @@
                           R13, R13_H,
                           R14, R14_H);

-// Class for all long registers except RAX (and RSP)
-reg_class long_no_rax_reg(RBP, RBP_H,
- RDX, RDX_H,
+// Class for all long registers except RAX (and RSP, RBP)
+reg_class long_no_rax_reg(RDX, RDX_H,
                           RDI, RDI_H,
                           RSI, RSI_H,
                           RCX, RCX_H,
@@ -325,10 +317,9 @@
// Singleton class for RDX long register
reg_class long_rdx_reg(RDX, RDX_H);

-// Class for all int registers (except RSP)
+// Class for all int registers (except RSP and RBP)
reg_class int_reg(RAX,
                   RDX,
- RBP,
                   RDI,
                   RSI,
                   RCX,
@@ -340,10 +331,9 @@
                   R13,
                   R14);

-// Class for all int registers except RCX (and RSP)
+// Class for all int registers except RCX (and RSP, RBP)
reg_class int_no_rcx_reg(RAX,
                          RDX,
- RBP,
                          RDI,
                          RSI,
                          RBX,
@@ -355,8 +345,7 @@
                          R14);

// Class for all int registers except RAX, RDX (and RSP)
-reg_class int_no_rax_rdx_reg(RBP,
- RDI,
+reg_class int_no_rax_rdx_reg(RDI,
                              RSI,
                              RCX,
                              RBX,
@@ -718,6 +707,7 @@
     st->print("# stack bang");
     st->print("\n\t");
     st->print("pushq rbp\t# Save rbp");
+ // BDG consider: st->print("movq rbp, rsp\t# ");
     if (framesize) {
       st->print("\n\t");
       st->print("subq rsp, #%d\t# Create frame",framesize);
--- openjdk8clean/hotspot/src/cpu/x86/vm/macroAssembler_x86.cpp 2014-03-04 02:52:11.000000000 +0000
+++ openjdk8/hotspot/src/cpu/x86/vm/macroAssembler_x86.cpp 2014-11-07 23:57:11.589593723 +0000
@@ -5236,6 +5236,7 @@
     // We always push rbp, so that on return to interpreter rbp, will be
     // restored correctly and we can correct the stack.
     push(rbp);
+ mov(rbp, rsp);
     // Remove word for ebp
     framesize -= wordSize;

--- openjdk8clean/hotspot/src/cpu/x86/vm/c1_MacroAssembler_x86.cpp 2014-03-04 02:52:10.000000000 +0000
+++ openjdk8/hotspot/src/cpu/x86/vm/c1_MacroAssembler_x86.cpp 2014-11-07 23:57:21.933257882 +0000
@@ -358,6 +358,7 @@
   generate_stack_overflow_check(frame_size_in_bytes);

   push(rbp);
+ mov(rbp, rsp);
#ifdef TIERED
   // c2 leaves fpu stack dirty. Clean it on entry
   if (UseSSE < 2 ) {

Show

Vladimir Kozlov added a comment - 2015-01-13 18:00 Recently we received request with proposed patch from Brendan Gregg: In case there's problems with the patch URL, the patch is: --- openjdk8clean/hotspot/src/cpu/x86/vm/x86_64.ad 2014-03-04 02:52:11.000000000 +0000 +++ openjdk8/hotspot/src/cpu/x86/vm/x86_64.ad 2014-11-08 01:10:49.686044933 +0000 @@ -166,10 +166,9 @@ // 3) reg_class stack_slots( /* one chunk of stack-based "registers" */ ) // -// Class for all pointer registers (including RSP) +// Class for all pointer registers (including RSP, excluding RBP) reg_class any_reg(RAX, RAX_H, RDX, RDX_H, - RBP, RBP_H, RDI, RDI_H, RSI, RSI_H, RCX, RCX_H, @@ -184,10 +183,9 @@ R14, R14_H, R15, R15_H); -// Class for all pointer registers except RSP +// Class for all pointer registers except RSP and RBP reg_class ptr_reg(RAX, RAX_H, RDX, RDX_H, - RBP, RBP_H, RDI, RDI_H, RSI, RSI_H, RCX, RCX_H, @@ -199,9 +197,8 @@ R13, R13_H, R14, R14_H); -// Class for all pointer registers except RAX and RSP +// Class for all pointer registers except RAX, RSP and RBP reg_class ptr_no_rax_reg(RDX, RDX_H, - RBP, RBP_H, RDI, RDI_H, RSI, RSI_H, RCX, RCX_H, @@ -226,9 +223,8 @@ R13, R13_H, R14, R14_H); -// Class for all pointer registers except RAX, RBX and RSP +// Class for all pointer registers except RAX, RBX, RSP and RBP reg_class ptr_no_rax_rbx_reg(RDX, RDX_H, - RBP, RBP_H, RDI, RDI_H, RSI, RSI_H, RCX, RCX_H, @@ -260,10 +256,9 @@ // Singleton class for TLS pointer reg_class ptr_r15_reg(R15, R15_H); -// Class for all long registers (except RSP) +// Class for all long registers (except RSP and RBP) reg_class long_reg(RAX, RAX_H, RDX, RDX_H, - RBP, RBP_H, RDI, RDI_H, RSI, RSI_H, RCX, RCX_H, @@ -275,9 +270,8 @@ R13, R13_H, R14, R14_H); -// Class for all long registers except RAX, RDX (and RSP) -reg_class long_no_rax_rdx_reg(RBP, RBP_H, - RDI, RDI_H, +// Class for all long registers except RAX, RDX (and RSP, RBP) +reg_class long_no_rax_rdx_reg(RDI, RDI_H, RSI, RSI_H, RCX, RCX_H, RBX, RBX_H, @@ -288,9 +282,8 @@ R13, R13_H, R14, R14_H); -// Class for all long registers except RCX (and RSP) -reg_class long_no_rcx_reg(RBP, RBP_H, - RDI, RDI_H, +// Class for all long registers except RCX (and RSP, RBP) +reg_class long_no_rcx_reg(RDI, RDI_H, RSI, RSI_H, RAX, RAX_H, RDX, RDX_H, @@ -302,9 +295,8 @@ R13, R13_H, R14, R14_H); -// Class for all long registers except RAX (and RSP) -reg_class long_no_rax_reg(RBP, RBP_H, - RDX, RDX_H, +// Class for all long registers except RAX (and RSP, RBP) +reg_class long_no_rax_reg(RDX, RDX_H, RDI, RDI_H, RSI, RSI_H, RCX, RCX_H, @@ -325,10 +317,9 @@ // Singleton class for RDX long register reg_class long_rdx_reg(RDX, RDX_H); -// Class for all int registers (except RSP) +// Class for all int registers (except RSP and RBP) reg_class int_reg(RAX, RDX, - RBP, RDI, RSI, RCX, @@ -340,10 +331,9 @@ R13, R14); -// Class for all int registers except RCX (and RSP) +// Class for all int registers except RCX (and RSP, RBP) reg_class int_no_rcx_reg(RAX, RDX, - RBP, RDI, RSI, RBX, @@ -355,8 +345,7 @@ R14); // Class for all int registers except RAX, RDX (and RSP) -reg_class int_no_rax_rdx_reg(RBP, - RDI, +reg_class int_no_rax_rdx_reg(RDI, RSI, RCX, RBX, @@ -718,6 +707,7 @@ st->print("# stack bang"); st->print("\n\t"); st->print("pushq rbp\t# Save rbp"); + // BDG consider: st->print("movq rbp, rsp\t# "); if (framesize) { st->print("\n\t"); st->print("subq rsp, #%d\t# Create frame",framesize); --- openjdk8clean/hotspot/src/cpu/x86/vm/macroAssembler_x86.cpp 2014-03-04 02:52:11.000000000 +0000 +++ openjdk8/hotspot/src/cpu/x86/vm/macroAssembler_x86.cpp 2014-11-07 23:57:11.589593723 +0000 @@ -5236,6 +5236,7 @@ // We always push rbp, so that on return to interpreter rbp, will be // restored correctly and we can correct the stack. push(rbp); + mov(rbp, rsp); // Remove word for ebp framesize -= wordSize; --- openjdk8clean/hotspot/src/cpu/x86/vm/c1_MacroAssembler_x86.cpp 2014-03-04 02:52:10.000000000 +0000 +++ openjdk8/hotspot/src/cpu/x86/vm/c1_MacroAssembler_x86.cpp 2014-11-07 23:57:21.933257882 +0000 @@ -358,6 +358,7 @@ generate_stack_overflow_check(frame_size_in_bytes); push(rbp); + mov(rbp, rsp); #ifdef TIERED // c2 leaves fpu stack dirty. Clean it on entry if (UseSSE < 2 ) {

9 older comments

Hide

Permalink

Zoltan Majo added a comment - 2015-04-29 23:29

Thanks, David, for pointing that out!

Show

Zoltan Majo added a comment - 2015-04-29 23:29 Thanks, David, for pointing that out!

Hide

Permalink

HG Updates added a comment - 2015-05-13 14:38

URL: http://hg.openjdk.java.net/jdk9/jdk9/hotspot/rev/382e9e4b3b71
User: lana
Date: 2015-05-13 21:19:48 +0000

Show

HG Updates added a comment - 2015-05-13 14:38 URL: http://hg.openjdk.java.net/jdk9/jdk9/hotspot/rev/382e9e4b3b71 User: lana Date: 2015-05-13 21:19:48 +0000

Hide

Permalink

Ilya Dergalin added a comment - 2015-05-21 08:04

Was performance testing done for these changes?

Show

Ilya Dergalin added a comment - 2015-05-21 08:04 Was performance testing done for these changes?

Hide

Permalink

Zoltan Majo added a comment - 2015-05-21 08:07

Hi Ilya,

yes, Robert Strout tested performance, please see below:

"Sorry for the delay on getting the performance results... We've had most of
the results for awhile, but one of the SPARC machines has been down which
held up some of the results. Machine is still down, and not sure when it's
coming back online. Those impacted results show in the results table as "Failed"...

http://aurora.se.oracle.com/performance/reporting/report/robert.strout.JDK-8068945.Compare

But we can ignore those since we don't need the SPARC results for this anyway.
I can assess the performance results as follows...

    Overall, most of the benchmarks in Aurora.se shown no definitive change.

    There's strong evidence that this benchmark regresses on all x64 platforms:
            SPECjvm2008-Compress* -2% to -3.5%

    There's evidence that this benchmark regresses on all x64 platforms:
            SPECjvm2008-Derby* -2% to -5%

Normally, I would follow up with a run of derby with more data points to get
a stronger measurement for it. And also do the same for a few weak results just
to make sure something is not hiding there. But given this change is under a
flag, I don't think we need to take this further. If you were going to consider
making this the default, then I'd want to run all the SPECjvm2008 benchmarks with
more iterations to better understand any impact it might have on the default.

--Resii"

PreserveFramePointer is off by default, so this change has no impact on VM performance in the default configuration.

Best regards,

Zoltan

Show

Zoltan Majo added a comment - 2015-05-21 08:07 Hi Ilya, yes, Robert Strout tested performance, please see below: "Sorry for the delay on getting the performance results... We've had most of the results for awhile, but one of the SPARC machines has been down which held up some of the results. Machine is still down, and not sure when it's coming back online. Those impacted results show in the results table as "Failed"... http://aurora.se.oracle.com/performance/reporting/report/robert.strout.JDK-8068945.Compare But we can ignore those since we don't need the SPARC results for this anyway. I can assess the performance results as follows... Overall, most of the benchmarks in Aurora.se shown no definitive change. There's strong evidence that this benchmark regresses on all x64 platforms: SPECjvm2008-Compress* -2% to -3.5% There's evidence that this benchmark regresses on all x64 platforms: SPECjvm2008-Derby* -2% to -5% Normally, I would follow up with a run of derby with more data points to get a stronger measurement for it. And also do the same for a few weak results just to make sure something is not hiding there. But given this change is under a flag, I don't think we need to take this further. If you were going to consider making this the default, then I'd want to run all the SPECjvm2008 benchmarks with more iterations to better understand any impact it might have on the default. --Resii" PreserveFramePointer is off by default, so this change has no impact on VM performance in the default configuration. Best regards, Zoltan

Hide

Permalink

David Therkelsen added a comment - 2015-05-27 13:16

Request for post-FC 8u60 backport for 8068945, 8075798 and 8080281 presented and approved at 5/27 RT mtg.

Show

David Therkelsen added a comment - 2015-05-27 13:16 Request for post-FC 8u60 backport for 8068945, 8075798 and 8080281 presented and approved at 5/27 RT mtg.

Use RBP register as proper frame pointer in JIT compiled code on x86

Details

Backports

Description

Issue Links

Sub-Tasks

Activity

People

Dates

Agile