Re: [AArch64] Fix simd intrinsics bug on float vminnm/vmaxnm

[Kyrill Tkachov <kyrylo dot tkachov at foss dot arm dot com>]

Hi all,

On 06/07/16 16:29, James Greenhalgh wrote:
> On Wed, Jul 06, 2016 at 02:11:51PM +0100, Jiong Wang wrote:
> > The current vmaxnm/vminnm float intrinsics are implemented using
> > __builtin_aarch64_smax/min<mode>  which are mapping to backend patterns
> > using smin/smax rtl operators.  However as documented in rtl.def
> >
> >    "Further, if both operands are zeros, or if either operand is NaN, then
> >    it is unspecified which of the two operands is returned as the result."
> >
> > There is no guarantee that a number will always be returned through
> > smin/smax operator, and further tests show gcc will optimize something
> > like smin (1.0f, Nan) to Nan, so current the vmaxnm and vminnm intrinsics
> > will evetually fail the new added testcases included in this patch.
> >
> > This patch:
> >
> >    * Migrate vminnm/vmaxnm float intrinsics to "<fmaxmin><mode>3" pattern
> >      which guarantee fminnm/fmaxnm sematics.
> >
> >    * Add new testcases for vminnm and vmaxnm intrinsics which were missing
> >      previously.  They are marked as XFAIL on arm*-*-* as ARM hasn't
> >      implemented these intrinsics.
> >
> > OK for trunk?
> The AArch64 parts are OK. I can't remember whether the ARM port prefers
> to have missing intrinsics XFAIL'd or if there is another way to disable
> the tests that are not supported there. Kyrill/Christophe would you mind
> commenting on whether this patch is correct for the intrinsics testsuite?
>
> Thanks,
> James
>   
> > 2016-07-06  Jiong Wang  <jiong.wang@arm.com>
> >
> > gcc/
> >    * config/aarch64/aarch64-simd-builtins.def (smax): Remove float variants.
> >    (smin): Likewise.
> >    (fmax): New entry.
> >    (fmin): Likewise.
> >    * config/aarch64/arm_neon.h (vmaxnm_f32): Use __builtin_aarch64_fmaxv2sf.
> >    (vmaxnmq_f32): Likewise.
> >    (vmaxnmq_f64): Likewise.
> >    (vminnm_f32): Likewise.
> >    (vminnmq_f32): Likewise.
> >    (vminnmq_f64): Likewise.

These intrinsics are supposed to be available for arm as well *except* for
vminnmq_f64, vmaxnmq_f64.

For the intrinsics that should be implemented but aren't can you please file a bug report.?
I see your patch doesn't xfail the test on arm, just skips it (so it appears unsupported).

My preferred course of action is to guard the vminmq_f64, vmaxnmq_f64 parts of the test
with #ifdef __aarch64__ and xfail the whole test for arm, with something like this:

{ dg-xfail-if "Intrinsics not yet implemented on arm <number of bugzilla PR>" { arm*-*-* } }

I do believe an XFAIL is appropriate here as the intrinsics should exist on arm, but don't
currently due to a missed-implementation bug.

Thanks,
Kyrill

> > gcc/testsuite/
> >    * gcc.target/aarch64/advsimd-intrinsics/binary_op_no64.inc: Support HAS_INTEGER_VARIANT.
> >    * gcc.target/aarch64/advsimd-intrinsics/vrhadd.c: Define HAS_INTEGER_VARIANT.
> >    * gcc.target/aarch64/advsimd-intrinsics/vhadd.c: Define HAS_INTEGER_VARIANT.
> >    * gcc.target/aarch64/advsimd-intrinsics/vhsub.c: Define HAS_INTEGER_VARIANT.
> >    * gcc.target/aarch64/advsimd-intrinsics/vmax.c: Define HAS_INTEGER_VARIANT.
> >    * gcc.target/aarch64/advsimd-intrinsics/vmin.c: Define HAS_INTEGER_VARIANT.
> >    * gcc.target/aarch64/advsimd-intrinsics/vhadd.c: Define HAS_INTEGER_VARIANT.
> >    * gcc.target/aarch64/advsimd-intrinsics/vmaxnm.c: New.
> >    * gcc.target/aarch64/advsimd-intrinsics/vminnm.c: New.