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

Tue Nov 24 18:22:15 UTC 2015

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