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 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?

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]