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

Thu Dec 3 06:32:44 UTC 2015

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%28do
>> 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
>>>>>