[aarch64-port-dev ] [PATCH] 8217561 : X86: Add floating-point Math.min/max intrinsics, approval request

[Andrew Dinn]

Hi Pengfei,

On 04/03/2019 10:06, Pengfei Li (Arm Technology China) wrote:
> 
> Thanks for your work on testing my pending patch. But I have to say
> the 3rd implementation (vecplus) is buggy. The main reason I didn't
> use FMAXV/FMINV for 2 doubles is the architecture doesn't support
> that.
> . . .
> So if you use "__ fmaxv(as_FloatRegister($dst$$reg), __ T2D,
> as_FloatRegister($src2$$reg));" in the ad file. The code generated
> will still be something like "fmaxv     s21, v16.4s" (with 4S
> arrangement). And if you run the test with fastdebug build JDK, the
> assertion failure will be hit.

Thank you for pointing this out. I had not noticed the warning about
this encoding being reserved in the ARM ARM nor that your encoding was
forcing the Q and sz bits to be 1 0.

What seems very odd to me is the difference between fmaxv and fminv.
Both Q == 1 encodings (i.e. with sz in {0, 1}) are reserved for fmaxv.
However, the encoding for fminv accepts both Q == 1 encodings with the
expected interpretation.

> Maybe I shouldn't hard-code the arrangement bits in the encoding as
> it may hide problems. I will modify my previous webrev and post it in
> a new thread if you think so.

Yes, I think it would probably be better to leave the assert in place
and use the encoding implied by the SIMD_Arrangement parameter i.e. T2S
==> Q=1,sz=0 and T2D ==> Q=1, sz=1. That way the assert will catch
errors in debug builds and non-debug builds should be stopped by a
SIGILL exception.

Also, if you adjust the assert to only apply to the fmax case then we
can still translate 2D FMIN reduction using the fminv instruction.

  assert(opc == 1 || T == T4S, "fmaxv arrangement must be T4S");

regards,


Andrew Dinn
-----------
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander