This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [AArch64] Fix simd intrinsics bug on float vminnm/vmaxnm
- From: Christophe Lyon <christophe dot lyon at linaro dot org>
- To: James Greenhalgh <james dot greenhalgh at arm dot com>
- Cc: Jiong Wang <jiong dot wang at foss dot arm dot com>, GCC Patches <gcc-patches at gcc dot gnu dot org>, nd <nd at arm dot com>, Kyrylo Tkachov <kyrtka01 at arm dot com>
- Date: Wed, 6 Jul 2016 17:41:43 +0200
- Subject: Re: [AArch64] Fix simd intrinsics bug on float vminnm/vmaxnm
- Authentication-results: sourceware.org; auth=none
- References: <f8a47ab3-ee52-4964-79b8-199e0562152e@foss.arm.com> <20160706152911.GA28331@arm.com>
On 6 July 2016 at 17:29, James Greenhalgh <james.greenhalgh@arm.com> 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?
>
Are they really XFAIL? The patch has dg-skip-if "arm*-*-*".
FWIW, there are currently 2 tests with such a dg-skip-if directive.
Other tests which depend on the target have:
dg-require-effective-target arm_crypto_ok
dg-require-effective-target arm_neon_fp16_hw { target { arm*-*-* } }
dg-require-effective-target arm_v8_1a_neon_hw
So I think the dg-skip-if directive this patch contains is OK.
Christophe
> 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.
>>
>> 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.
>