RFR: 8189105: AARCH64: create intrinsic for sin and cos

Dmitrij Pochepko dmitrij.pochepko at bell-sw.com
Wed Jun 20 14:26:23 UTC 2018



On 19.06.2018 20:46, Andrew Haley wrote:
> On 06/18/2018 07:53 PM, Dmitrij Pochepko wrote:
>> If you have any other ideas how to make the code easier to read and
>> maintain I’m happy to improve it.
> That's great!  Really, that's a vast improvement.  Excellent work.
>
> One or two relatively minor things:
>
> Why do you say the polynomial in kernel_sin/kernel_cos is a Taylor
> series?  There's no such comment in sharedRuntimeTrig.cpp, and I doubt
> it.  I would have thought it was a
> least-maximum polynomial
> approximation, not a Taylor series.
Taylor series for cos(x) = 1 - x^2/2! + x^4/4! - x^6/6! + ... = 1 - 
0.5*x^2 + 4.16666666666666019037e-02 * x^4 - 1.38888888888741095749e-03 
* x^6 ...
so, if you take a look at kernel_cos function description you'll see 
exactly same polynomial calculation with exactly same coefficients.
>
> What's the point of keeping the doubles in tables in hex form?
> sharedRuntimeTrig.cpp doesn't do that, and it looks like obfuscation.
> Even if you must keep them as hex, at least move the comment which
> shows their true values next to their declaration, but I really can't
> see the point.
It was easier for me while debugging to use hex form. I changed 
representation to double with hex form added in comment where applicable.
>
> It'd make the code much more readable if you lined up the comments,
> like so:
>
>              fmovd(v28, v3);           // t = r
>              fmuld(v29, v2, v6);       // w = v29 = fn * pio2_2
>              fsubd(v3, v28, v29);      // r = t - w
>              fsubd(v31, v28, v3);      // v31 = (t - r)
>              fsubd(v31, v29, v31);     // v31 = w - (t - r) = - ((t - r) - w)
>              fmaddd(v26, v2, v7, v31); // v26 = w = fn*pio2_2t - ((t - r) - w)
>              fsubd(v4, v3, v26);       // y[0] = r - w
>              fmovd(jx, v4);
>              lsl(jx, jx, 1);
>              sub(tmp3, tmp5, jx, LSR, 32 + 20 + 1); // r7 = j-(((*(i0+(int*)&y[0]))>>20)&0x7ff);
>
> But it's probably not a great idea to make the code-super wide with
> the comments so you have to open a huge editor window.  So, don't
> go crazy, but a little bit of white space like this makes it much
> easier to read.
done
>
> Thanks.
>


Please take a look at updated version:
http://cr.openjdk.java.net/~dpochepk/8189105/webrev.04/

Thanks,
Dmitrij


More information about the hotspot-compiler-dev mailing list