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

Vladimir Kozlov vladimir.kozlov at oracle.com
Thu Dec 3 21:25:25 UTC 2015 

Vivek,

I think Joe is asking you to write these tests as hotspot regression test in hotspot/test/compiler.

Vladimir

On 12/3/15 1:22 PM, Deshpande, Vivek R wrote:
> 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