This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


On 7 July 2016 at 11:16, Jiong Wang <jiong.wang@foss.arm.com> wrote:
> On 06/07/16 16:55, Christophe Lyon wrote:
>>
>> On 6 July 2016 at 17:44, Kyrill Tkachov <kyrylo.tkachov@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.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.
>>>
>> 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
>
> I was using dg-xfail-if, (the description is still using "marked as
> XFAIL"...),
> but later found it's actually broken under advsimd-intrinsics, UNRESOLVEDs
> are
> given at the same time instead of clean XFAIL, I suspect those dg-do-what
> overriding broken dejangu internal variable, Christophe, do you mind have a
> look?
>
I've made a quick attempt (replacing an existing dg-skip-if with
dg-xfail-if in vcvt_high-1.c)
and I do see XFAIL and UNRESOLVED.
But this seems normal in this case, because:
- when using dg-skip-if, the test was not compiled (skipped)
- when using dg-xfail-if, the test is actually compiled, leading to
compilation errors which are reported as UNRESOLVED

> Meanwhile, as the new vminnm and vmaxnm testcases are using the existed test
> infrastructure of vmin/vmax infrastructure which is based on
> binary-op-no64.inc
> where only float32 defined.  If we enable float64x2, there will be two
> issues:
>
>   * The naming of binary-iop-no64.inc needs to be updated, but it's used by
>     several other files, so not sure it's the correct approach.
>
>   * You will want to xfail only float64x2 testing on ARM inside vmin.c and
>     vmax.c, I don't know how to do that.
>
> For the vminnm and vmaxnm testing, I really want to go ahead to implement
> them
> for ARM so we won't be bothered by xfail, there is backend pattern already
> which
> is fmin/fax, however they are standard name without "neon_" prefix, while
> unlike
> AArch64, ARM neon builtins infrastructure force the prefix to be "neon_".
> The
> macro expand infrastructure needs to be updated.
>
> For this patch, I am going to change dg-skip-if to dg-xfail-if, but please
> be
> prepare with those UNRESOLVED failures which will go away once
> advsimd-intrinsics.exp fixed.  Meanwhile I will create seperate test file
> for
> float64x2, and dg-skip them on ARM.
>
>


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]