On 6 July 2016 at 17:44, Kyrill Tkachov <kyrylo.tkac...@foss.arm.com> wrote:
> 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.w...@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.
>
I missed that point.
So, I agree with Kyrill:
- skip the ones that aren't supposed to be available for arm
- xfail the ones that aren't implemented yet.
Christophe
> 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.
>
>
