RFR (M): 8143353: Update for x86 sin and cos in the math lib

Deshpande, Vivek R vivek.r.deshpande at intel.com
Thu Dec 3 21:22:52 UTC 2015 

Hi Joe

It would be great if you would please share the additional tests with us.

Regards,
Vivek

-----Original Message-----
From: joe darcy [mailto:joe.darcy at oracle.com] 
Sent: Thursday, December 03, 2015 1:17 PM
To: Vladimir Kozlov; Deshpande, Vivek R
Cc: Viswanathan, Sandhya; Berg, Michael C; hotspot compiler
Subject: Re: RFR (M): 8143353: Update for x86 sin and cos in the math lib

I think it is unwise for this large of an implementation change to be pushed with no tests targeting the specifics of the new implementation.

The worst-case tests in the jdk repo are the mathematical worst cases for floating-point approximations, in other words the cases were the exact mathematical answer is closes to half-way between two representation floating-point numbers. Passing such tests is necessary but not sufficient condition for a new implementation.

Chers,

-Joe

On 12/3/2015 1:05 PM, Vladimir Kozlov wrote:
> Okay, looks reasonable to me.
>
> Thanks,
> Vladimir
>
> On 12/3/15 11:06 AM, Deshpande, Vivek R wrote:
>> Hi Vladimir
>>
>> This is the link for the updated webrev with latest hotspot source as 
>> base for your review.
>> http://cr.openjdk.java.net/~mcberg/8143353/webrev.03/
>> Thank you.
>>
>> Regards,
>> Vivek
>>
>> -----Original Message-----
>> From: Deshpande, Vivek R
>> Sent: Wednesday, December 02, 2015 10:33 PM
>> To: 'Vladimir Kozlov'; joe darcy
>> Cc: Viswanathan, Sandhya; Berg, Michael C; hotspot compiler
>> Subject: RE: RFR (M): 8143353: Update for x86 sin and cos in the math 
>> lib
>>
>> Hi Vladimir
>>
>> This is the link for the updated webrev for your review.
>> http://cr.openjdk.java.net/~mcberg/8143353/webrev.02/
>> Thank you.
>>
>> Regards,
>> Vivek
>>
>>
>>
>> -----Original Message-----
>> From: Vladimir Kozlov [mailto:vladimir.kozlov at oracle.com]
>> Sent: Tuesday, December 01, 2015 6:06 PM
>> To: Deshpande, Vivek R; joe darcy
>> Cc: Viswanathan, Sandhya; Berg, Michael C; hotspot compiler
>> Subject: Re: RFR (M): 8143353: Update for x86 sin and cos in the math 
>> lib
>>
>> Please send link to new webrev on cr server.
>>
>> Thanks,
>> Vladimir
>>
>> On 11/25/15 5:16 PM, Deshpande, Vivek R wrote:
>>> Hi Vladimir
>>>
>>> Please find the webrev with your suggested updates attached with the 
>>> mail.
>>> We will update it in the jbs entry soon.
>>> Please let me know if it needs further changes.
>>>
>>> Regards,
>>> Vivek
>>>
>>> -----Original Message-----
>>> From: Deshpande, Vivek R
>>> Sent: Tuesday, November 24, 2015 10:22 AM
>>> To: 'joe darcy'; Vladimir Kozlov
>>> Cc: Viswanathan, Sandhya; Berg, Michael C; hotspot compiler
>>> Subject: RE: RFR (M): 8143353: Update for x86 sin and cos in the 
>>> math lib
>>>
>>> HI Vladimir, Joe
>>>
>>> I have done the jtreg tests in hotspot and tests from jdk you have 
>>> mentioned. It passed those tests.
>>> The ~4x gain is with XX:+UnlockDiagnosticVMOptions 
>>> -XX:DisableIntrinsic=_dsin/_dcos over without that option.
>>> The performance gain is 3.2x over base jdk, that is over current 
>>> fsin/fcos intrinsic. This gain is more realistic.
>>>
>>> Could I get those tests around the boundary values. Would 
>>> WorstCaseTests.java jtreg test in jdk test those ?
>>> If yes, then it has passed those boundary cases.
>>>
>>> I would work on adding either diagnostic flag or just one flag for 
>>> libm and send out the webrev soon.
>>>
>>> Regards,
>>> Vivek
>>>
>>>
>>> -----Original Message-----
>>> From: joe darcy [mailto:joe.darcy at oracle.com]
>>> Sent: Monday, November 23, 2015 6:28 PM
>>> To: Vladimir Kozlov; Deshpande, Vivek R
>>> Cc: Viswanathan, Sandhya; Berg, Michael C; hotspot compiler
>>> Subject: Re: RFR (M): 8143353: Update for x86 sin and cos in the 
>>> math lib
>>>
>>> Hello,
>>>
>>> Just getting added to the thread..
>>>
>>> On 11/23/2015 5:13 PM, Vladimir Kozlov wrote:
>>>> Thank you, for explanation, Vivek.
>>>>
>>>> Please, run jdk/test/java/lang/Math/ jtreg tests in addition to 
>>>> Hotspot tests.
>>>>
>>>> On 11/23/15 12:24 PM, Deshpande, Vivek R wrote:
>>>>> Hi Vladimir
>>>>>
>>>>> The result we obtain with LIBM are within +/- 1ulp from StrictMath 
>>>>> result and not exact result. So I added the flag to switch between 
>>>>> FDLIBM and LIBM.
>>>>>
>>>>> Quick explanation:
>>>>> This is what we observed with comparison to HPA Library
>>>>> (http://www.nongnu.org/hpalib/) explained with an example.
>>>>> LIBM Observed Math result=0.19457293629570213 
>>>>> (4596178249117717083L) (StrictMath - 1ulp) Required result should 
>>>>> be = 0.19457293629570216
>>>>> (4596178249117717084L) (StrictMath result) or 0.1945729362957022
>>>>> (4596178249117717085L) (StrictMath + 1ulp.) This means HPA library 
>>>>> result is between the above two values and Exact result would be 
>>>>> pretty close to it.
>>>>> So here StrictMath result is less than quad-precision result, Math 
>>>>> result should be StrictMath or StrictMath + 1ulp and not 
>>>>> StrictMath
>>>>> - 1ulp, according to our test.
>>>>
>>>> Note, java.lang.Math allows to have 1ulp off (in both direction, I
>>>> think) and it should be consistent for Interpreter and code 
>>>> generated by JIT compilers:
>>>>
>>>> http://docs.oracle.com/javase/7/docs/api/java/lang/Math.html#sin%28
>>>> do
>>>> u
>>>> ble%29
>>>>
>>>
>>> That interpretation of the spec is not quite right. For the Math 
>>> methods with a 1/2 ulp error bound, the floating-point result 
>>> closest to the exact result must be returned. For the methods with a
>>> 1 ulp error bound, either of the floating-point result bracketing 
>>> the true result can be returned, subject to the monotonicity 
>>> constraints of the specification of the particular method.
>>>
>>>>
>>>>>
>>>>> I have done the experiments with XX:+UnlockDiagnosticVMOptions 
>>>>> -XX:DisableIntrinsic=_dsin and XX:+UnlockDiagnosticVMOptions 
>>>>> -XX:DisableIntrinsic=_dcos. With this option, the interpreter 
>>>>> would go through LIBM and C1 and c2 through FDLIBM.
>>>>> If we want to disable LIBM completely, we need the flags 
>>>>> -XX:+UseLibmSinIntrinsic and -XX:+UseLibmCosIntrinsic.
>>>>
>>>> I was thinking about using existing
>>>> DirectiveSet::is_intrinsic_disabled() and 
>>>> vmIntrinsics::is_disabled_by_flags(). You need to add additional 
>>>> versions of functions which accept intrinsic ID instead of 
>>>> methodHandle.
>>>>
>>>> If you still want to use flags make them diagnostic.
>>>> Or have one flag for all LIBM intrinsics -XX:+UseLibmIntrinsic.
>>>>
>>>>>
>>>>> Also the performance gain ~4x is with 
>>>>> XX:+UnlockDiagnosticVMOptions -XX:DisableIntrinsic=_dsin/_dcos.
>>>>
>>>> You confused me here. So you get 4x when only Interpreter use LIBM 
>>>> code and compilers use FDLIB?
>>>
>>> Just to be clear, are you comparing the new code to FDLIBM
>>> (StrictMath) or to the existing fsin/fcos instrinsics (Math)?
>>>
>>> I'm part way through porting the FDLIBM code to Java (JDK-8134780: 
>>> Port fdlibm to Java), which is providing a significant speed boost 
>>> to the StrictMath methods that have been ported.
>>>
>>> I find the current patch *insufficient* as-is in terms of its testing.
>>> For example, part of patch says
>>>
>>> # For sin
>>>
>>> +//     This means that the main path is actually only taken for
>>> +//     2^-252 <= |X| < 90112.
>>>
>>> # For cos
>>>
>>> +//     This means that the main path is actually only taken for
>>> +//     2^-252 <= |X| < 90112.
>>>
>>> If nothing else, there are no tests at around those boundary values, 
>>> which is unacceptable. There should also be some tests of values of 
>>> interest to the algorithm in question.
>>>
>>> Cheers,
>>>
>>> -Joe
>>>
>>>
>>>>
>>>> Thanks,
>>>> Vladimir
>>>>
>>>>>
>>>>> Let me know your thoughts on this. I would answer more questions 
>>>>> and give more data if needed.
>>>>>
>>>>> Regards,
>>>>> Vivek
>>>>>
>>>>>
>>>>> -----Original Message-----
>>>>> From: Vladimir Kozlov [mailto:vladimir.kozlov at oracle.com]
>>>>> Sent: Monday, November 23, 2015 10:37 AM
>>>>> To: Deshpande, Vivek R; hotspot-compiler-dev at openjdk.java.net
>>>>> Cc: Viswanathan, Sandhya
>>>>> Subject: Re: RFR (M): 8143353: Update for x86 sin and cos in the 
>>>>> math lib
>>>>>
>>>>> On 11/20/15 12:22 PM, Vladimir Kozlov wrote:
>>>>>> What is the reason you decided to add new flags? exp() and log() 
>>>>>> changes did not have flags.
>>>>>>
>>>>>> It would be interesting to see what happens if you disable 
>>>>>> intrinsics using existing flag, for example:
>>>>>>
>>>>>>      -XX:+UnlockDiagnosticVMOptions -XX:DisableIntrinsic=_dexp
>>>>>
>>>>> Hi Vivek,
>>>>>
>>>>> I want to point that you can do this experiment later. We can file 
>>>>> bugs and fixed them after FC.
>>>>>
>>>>> For now, please, answer my question about flags only. This is the 
>>>>> only thing holding it from push.
>>>>>
>>>>> Thanks,
>>>>> Vladimir
>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>> Vladimir
>>>>>>
>>>>>> On 11/20/15 12:03 PM, Deshpande, Vivek R wrote:
>>>>>>> Hi all
>>>>>>>
>>>>>>> I would like to contribute a patch which optimizes Math.sin() 
>>>>>>> and
>>>>>>> Math.cos() for 64 and 32 bit X86 architecture using Intel LIBM
>>>>>>>      implementation.
>>>>>>>
>>>>>>> The improvement gives ~4.25x gain over base for both sin and cos.
>>>>>>>
>>>>>>> The option to use the optimizations are -XX:+UseLibmSinIntrinsic 
>>>>>>> and -XX:+UseLibmCosIntrinsic.
>>>>>>>
>>>>>>> Could you please review and sponsor this patch.
>>>>>>>
>>>>>>> Bug-id:
>>>>>>>
>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8143353
>>>>>>> webrev:
>>>>>>>
>>>>>>> http://cr.openjdk.java.net/~mcberg/8143353/webrev.01/
>>>>>>>
>>>>>>> Thanks and regards,
>>>>>>>
>>>>>>> Vivek
>>>>>>>

More information about the hotspot-compiler-dev mailing list