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

Sat Jan 16 02:28:31 UTC 2016

Ah okay; I overlooked the separate push of the tests.

Thanks,

-Joe

On 1/15/2016 5:58 PM, Vladimir Kozlov wrote:
> Note, the test was pushed together with VM changes into hs-comp repo:
>
>  http://hg.openjdk.java.net/jdk9/hs-comp/jdk/rev/ddd59a780769
>
> New sin/cos code is tested in all running modes since it is used by 
> Interpreter and JITed code (C1 and C2).
>
> I will let Vivek answer questions about the test.
>
> Regards,
> Vladimir
>
> On 1/15/16 5:33 PM, Joseph D. Darcy wrote:
>> Hello,
>>
>> Catching up on email, how were these test cases generated or chosen? In
>> other words, in what sense are they corners?
>>
>> The data would be easier to read if the numbers were aligned by column
>> (they don't appear that way in the webrev at least).
>>
>> What is the code coverage of the new intrinsics with this set of tests?
>>
>> Theses tests should not be separated from the implementation for long;
>> in other words, since the new implementation has already been pushed to
>> a HotSpot forest, test coverage for that new implementation should not
>> lag behind.
>>
>> Thanks,
>>
>> -Joe
>>
>> On 12/22/2015 5:41 PM, Deshpande, Vivek R wrote:
>>> HI All
>>>
>>> I have uploaded the patch for sin and cos tests with input and allowed
>>> outputs
>>> at this location for your review.
>>> http://cr.openjdk.java.net/~vdeshpande/libm_sincos/8143353/jdk/webrev.00/ 
>>>
>>> Bug ID: https://bugs.openjdk.java.net/browse/JDK-8143353
>>> Thank you.
>>>
>>> Regards,
>>> Vivek
>>>
>>> -----Original Message-----
>>> From: Joseph D. Darcy [mailto:joe.darcy at oracle.com]
>>> Sent: Friday, December 04, 2015 4:50 PM
>>> To: Deshpande, Vivek R; 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 Vivek,
>>>
>>> On 12/3/2015 2:01 PM, Deshpande, Vivek R wrote:
>>>> Hi
>>>>
>>>> Sure I will add the tests. Shall I use StrictMath result as a
>>>> reference for exact result.
>>>> Let me know your thoughts.
>>> As a rough test of another sin/cos implementation, StrictMath.{sin,
>>> cos} can be used a reference with the following caveat: there isn't an
>>> indication of which why the error is in a StrictMath result. Let me
>>> given an example, if
>>>
>>>       StrictMath.sin(x) => y
>>>
>>> then one of the following should be true
>>>
>>>       Math.sin(x) => y
>>>       Math.sin(x) => Math.nextUp(y)
>>>       Math.sin(x) => Math.nextDown(y)
>>>
>>> That is, Math.sin(x) should either be the same as StrictMath.sin(x) OR
>>> equal to one of the floating-point numbers adjacent to that result. Of
>>> these three options, only two area allowed by the accuracy
>>> requirements of the StrictMath.sin specification. However, since
>>> StrictMath.sin doesn't give an indication of which way its error went
>>> (if it rounded up or down), there is no indication without additional
>>> work which of
>>> nextUp(y) and nextDown(y) is allowable (assuming StrictMath.sin isn't
>>> buggy).
>>>
>>> HTH,
>>>
>>> -Joe
>>>
>>>
>>>> Regards,
>>>> Vivek
>>>>
>>>> -----Original Message-----
>>>> From: joe darcy [mailto:joe.darcy at oracle.com]
>>>> Sent: Thursday, December 03, 2015 1:29 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,
>>>>
>>>> On 12/3/2015 1:25 PM, Vladimir Kozlov wrote:
>>>>> Vivek,
>>>>>
>>>>> I think Joe is asking you to write these tests as hotspot regression
>>>>> test in hotspot/test/compiler.
>>>> Exactly; if not generally applicable sin/cos tests that could be
>>>> hosted in the jdk repo (alongside the regression and unit tests for
>>>> java.lang.Math), then test of intrinsics in the HotSpot repo
>>>> alongside other tests targeting intrinsics.
>>>>
>>>> Thanks,
>>>>
>>>> -Joe
>>>>
>>>>> 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
>>>>>>>>>>>>>
>>