[9] RFR(S): 8068945: Use RBP register as proper frame pointer in JIT compiled code on x86
Vladimir Kozlov
vladimir.kozlov at oracle.com
Mon Mar 30 20:48:50 UTC 2015
Also change flag's comment in globals.hpp because it contradicts to the
code:
+ "Use the FP register as a general purpose register "
\
+ "and not for holding the frame pointer.")
+ if (FramePointer) {
+ mov(rbp, rsp);
+ }
Vladimir
On 3/30/15 1:27 PM, Vladimir Kozlov wrote:
> How about PreserveFramePointer instead of simple FramePointer?
>
> PreserveFramePointer will mean that compiled (or other) code will use
> that register only as Frame pointer.
>
> Zoltan, x86 flags setting should be in general globals_x86.hpp. You can
> #ifdef _LP64 there too. I don't understand why you only set it to true
> on linux-x64.
>
> Thanks,
> Vladimir
>
> On 3/30/15 8:30 AM, Zoltán Majó wrote:
>> Hi Ed,
>>
>>
>> thank you for your feedback! Please see comments below.
>>
>> On 03/30/2015 11:11 AM, Edward Nevill wrote:
>>> Hi Zoltán,
>>>
>>> On Fri, 2015-03-27 at 15:34 +0100, Zoltán Majó wrote:
>>>> Full JPRT run, all tests pass. I also ran all hotspot compiler tests
>>>> and
>>>> the jdk tests in java/lang/invoke on both x86_64 and x86_32. All tests
>>>> that pass without the patch pass also with the patch.
>>>>
>>>> I ran the SPEC JVM 2008 benchmarks on our performance infrastructure
>>>> for
>>>> x86_64. The performance evaluation suggests that there is no
>>>> statistically significant performance degradation due to having proper
>>>> frame pointers. Therefore I propose to have OmitFramePointer set to
>>>> false by default on x86_64 (and set to true on all other platforms).
>>> This patch looks good, however I think there is a problem with the
>>> logic of OmitFramePointer.
>>>
>>> Here is my test case.
>>>
>>> --- CUT HERE ---
>>> // $Id: fibo.java,v 1.2 2000/12/24 19:10:50 doug Exp $
>>> // http://www.bagley.org/~doug/shootout/
>>>
>>> public class fibo {
>>> public static void main(String args[]) {
>>> int N = Integer.parseInt(args[0]);
>>> System.out.println(fib(N));
>>> }
>>> public static int fib(int n) {
>>> if (n < 2) return(1);
>>> return( fib(n-2) + fib(n-1) );
>>> }
>>> }
>>> --- CUT HERE ---
>>>
>>> If I run it as follows on my x86 64 bit linux.
>>>
>>> /work/images/jdk/bin/java -XX:-TieredCompilation -XX:+PrintCompilation
>>> -XX:CompileOnly=fibo::fib -XX:+UnlockDiagnosticVMOptions
>>> -XX:-OmitFramePointer -XX:+PrintAssembly fibo 43
>>>
>>> I get
>>>
>>> # {method} {0x00007fc62c97f388} 'fib' '(I)I' in 'fibo'
>>> # parm0: rsi = int
>>> # [sp+0x30] (sp of caller)
>>> 0x00007fc625071100: mov %eax,-0x14000(%rsp)
>>> 0x00007fc625071107: push %rbp
>>> 0x00007fc625071108: mov %rsp,%rbp
>>> 0x00007f836907110b: sub $0x20,%rsp ;*synchronization entry
>>>
>>> which is correct, it is NOT(-) OmitFramePointer, therefore it is using
>>> the frame pointer
>>>
>>> Now if I try just changing -XX:-OmitFramePointer to
>>> -XX:+OmitFramePointer in the above I get
>>>
>>> /work/images/jdk/bin/java -XX:-TieredCompilation -XX:+PrintCompilation
>>> -XX:CompileOnly=fibo::fib -XX:+UnlockDiagnosticVMOptions
>>> -XX:+OmitFramePointer -XX:+PrintAssembly fibo 43
>>>
>>> I get
>>>
>>> # {method} {0x00007f14d3c00388} 'fib' '(I)I' in 'fibo'
>>> # parm0: rsi = int
>>> # [sp+0x30] (sp of caller)
>>> 0x00007f14e1071100: mov %eax,-0x14000(%rsp)
>>> 0x00007f14e1071107: push %rbp
>>> 0x00007f14e1071108: sub $0x20,%rsp ;*synchronization entry
>>>
>>> which is correct, it is ID(+) OmitFramePointer, therefore it does not
>>> use a frame pointer.
>>>
>>> However, if I now delete the -XX:+/-OmitFramePointer altogether, IE
>>>
>>> /work/images/jdk/bin/java -XX:-TieredCompilation -XX:+PrintCompilation
>>> -XX:CompileOnly=fibo::fib -XX:+UnlockDiagnosticVMOptions
>>> -XX:+PrintAssembly fibo 43
>>>
>>> I get
>>>
>>> # {method} {0x00007f0c4b730388} 'fib' '(I)I' in 'fibo'
>>> # parm0: rsi = int
>>> # [sp+0x30] (sp of caller)
>>> 0x00007f0c75071100: mov %eax,-0x14000(%rsp)
>>> 0x00007f0c75071107: push %rbp
>>> 0x00007f0c75071108: sub $0x20,%rsp ;*synchronization entry
>>>
>>> It is not using a frame pointer which is the equivalent of
>>> -XX:+OmitFramePointer. However in your description above you say
>>>
>>>> Therefore I propose to have OmitFramePointer set to false by default
>>>> on x86_64 (and set to true on all other platforms).
>>> whereas OmitFramePointer actually seems to be set to true on x86_64
>>>
>>> I think the problem may be with the declaration and definition of
>>> OmitFramePointer in globals.hpp and globals_x86.hpp
>>>
>>> In globals.hpp it does
>>>
>>> product(bool, OmitFramePointer, true,
>>>
>>> In globals_x86.hpp it does
>>>
>>> LP64_ONLY(define_pd_global(bool, OmitFramePointer, false););
>>>
>>> I am not sure that you can mix product(...) and product_pd(...) like
>>> this, so I think it just ends up getting the default from the
>>> product(...).
>>
>> You are right, mixing product and product_pd does not make sense at all.
>> Thank you for doing additional testing and for drawing attention to the
>> problem.
>>
>> I updated the code to use product_pd and define_pd_global on all
>> relevant platforms.
>>
>>> Aside: In general, I do not like options which include a negative in
>>> them because I have to do a double think when I see something like,
>>> -XX:-OmitFramePointer, as in, it is omitting the frame pointer,
>>> therefore it is using a frame pointer. How about FramePointer so we
>>> have -XX:+FramePointer to say I want frame pointers and
>>> -XX:-FramePointer to say I don't.
>>
>> That is a good idea. Double negation is an unnecessary complication, so
>> I changed the name of the flag to FramePointer, just as you suggested.
>>
>>>
>>> I did some timing on the above 'fibo' test
>>>
>>> [ed at mylittlepony java]$ time /work/images/jdk/bin/java
>>> -XX:-OmitFramePointer fibo 43
>>> 701408733
>>>
>>> real 0m1.545s
>>> user 0m1.571s
>>> sys 0m0.015s
>>> [ed at mylittlepony java]$ time /work/images/jdk/bin/java
>>> -XX:+OmitFramePointer fibo 43
>>> 701408733
>>>
>>> real 0m1.504s
>>> user 0m1.527s
>>> sys 0m0.019s
>>>
>>> which is ~3% difference on this test case. On aarch64, I see ~7%
>>> difference on this test case.
>>
>> Thank you for the performance measurements!
>>
>>> With the above change to fix the logic of OmitFramePointer (and
>>> possible change its name) the patch looks good to me.
>>
>> Here is the updated webrev (the same webrev that was already included
>> into my reply to Roland):
>>
>> http://cr.openjdk.java.net/~zmajo/8068945/webrev.01/
>>
>>> I will prepare a mirror patch for aarch64.
>>
>> That would be great!
>>
>> Thank you and best regards,
>>
>>
>> Zoltán
>>
>>>
>>> All the best,
>>> Ed.
>>>
>>>
>>>
>>
More information about the hotspot-compiler-dev
mailing list