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

[joe darcy]

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