[aarch64-port-dev ] RFR(M): 8196402: AARCH64: create intrinsic for Math.log

[White, Derek]

Hi Dmitrij,

Looks good to me. Thanks! 
- Derek

> -----Original Message-----
> From: Dmitrij Pochepko [mailto:dmitrij.pochepko at bell-sw.com]
> Sent: Wednesday, June 20, 2018 10:26 AM
> To: Andrew Haley <aph at redhat.com>; White, Derek
> <Derek.White at cavium.com>
> Cc: aarch64-port-dev at openjdk.java.net
> Subject: Re: [aarch64-port-dev ] RFR(M): 8196402: AARCH64: create intrinsic
> for Math.log
> 
> External Email
> 
> Hi,
> 
> 
> On 20.06.2018 11:31, Andrew Haley wrote:
> > On 06/20/2018 12:45 AM, White, Derek wrote:
> >>   - Line 68: Could align to 64 bytes. We're going to load the first 64 bytes,
> may as well use one cache line.
> Agree. It'll be a bit better to use 64 bytes alignment. I changed it to
> 64 and tested the revised patch. I could not see a difference in synthetic
> benchmark, but single invocation might gain a small difference in case of L1
> miss.
> >>
> >>   - Lines 71, 77, 79. You could comment these lines with "_coeff", "_log2",
> and "_L_tbl", for clearer correspondence to x86 code.
> Added respective comment.
> >>
> >> - Line 204: I believe that we should be looking for equal to 0.0:
> >> 204 //      if (X = 0.0d) return -INFINITY;
> > Yes.  if (X == 0.0d).
> Thank you for noticing this typo. Assembly is correct. I fixed this comment.
> >> - The (float)(zeroExponent(X) code was fun.
> >>
> >> - Are there any concerns about denormalized inputs? I didn't see any
> obvious problem in the code pulling aaprt and adding in exponents, but
> there's nothing obvious in that code to me.
> > IEEE denormals don't usually require any special handling.
> There is no denormals-specific code in this algorithm.
> >
> >> - The RETURN_MINF_OR_NAN block can generate a wide range of
> signaling and non-signaling NaNs. I forget how signaling Nans are treated in
> the Java math mode - are they just treated as quiet NaNs or are they never
> supposed to appear?
> > Java doesn't care.  However, it is possible to use DoubleToRawLongBits
> > to extract some information, and this is fine.
> 
> I also aligned comments as in sin/cos patch and fixed the copyright.
> 
> Here, the original code has table in HEX and also the article has HEX
> representation, so I kept HEX in the table.
> 
> Please take a look at updated webrev:
> http://cr.openjdk.java.net/~dpochepk/8196402/webrev.06/
> 
> Thanks,
> Dmitrij