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

Thu Dec 3 21:05:58 UTC 2015

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