Re: [PATCH][ARM] PR target/71270 fix neon_valid_immediate for big-endian

[Kyrill Tkachov]

On 20/01/17 14:33, Richard Earnshaw (lists) wrote:

On 06/01/17 11:54, Kyrill Tkachov wrote:

Hi all,

In this wrong-code issue the RTL tries to load a const_vector:
(const_vector:V8QI [
 (const_int 1 [0x1])
 (const_int 0 [0])
 (const_int 1 [0x1])
 (const_int 0 [0])
 (const_int 1 [0x1])
 (const_int 0 [0])
 (const_int 1 [0x1])
 (const_int 0 [0])
 ])

into a NEON register. The logic for that is in neon_valid_immediate
which does a number of tricks
to decide what value and of what size to do a VMOV on to load the
correct vector immediate into the register.
It goes wrong on big-endian. On both big and little-endian this outputs:
vmov.i16d18, #0x1

This is wrong on big-endian. The vector layout has to be such as if
loaded from memory.
I've tried various approaches of fixing neon_valid_immediate to generate
the correct immediate but have been unsuccessful,
resulting in regressions in various parts of the testsuite or making a
big mess of the function.

Given that armeb is not a target of major concern I believe the safest
route at this stage is to only allow vector constants
that will obviously work on big-endian, that is the ones that are just a
single element duplicated in all lanes.

This patch fixes the execution failures:
FAIL: gfortran.dg/intrinsic_pack_1.f90
FAIL: gfortran.dg/c_f_pointer_logical.f03
FAIL: gcc.dg/torture/pr60183.c
FAIL: gcc.dg/vect/pr51581-4.c

on armeb-none-eabi.

Ok for trunk?

Thanks,
Kyrill

2017-01-06  Kyrylo Tkachov  

 PR target/71270
 * config/arm/arm.c (neon_valid_immediate): Reject vector constants
 in big-endian mode when they are not a single duplicated value.


Ok, but if you plan to close the PR above, please create a new
'enhancement' PR to fix the missed optimization.


Thanks, I've committed it as r244716 and opened PR 79166 to track
the missed optimisation.

Kyrill


R.


armeb-vec.patch


diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
index 
45e0a3cd11fa650f456f7382ffbbcd1c932b95eb..2beee8ebe94eeadd6fb1d5b119e3b474e1ab902a
 100644
--- a/gcc/config/arm/arm.c
+++ b/gcc/config/arm/arm.c
@@ -11542,6 +11542,12 @@ neon_valid_immediate (rtx op, machine_mode mode, int 
inverse,
return 18;
  }
  
+  /* The tricks done in the code below apply for little-endian vector layout.

+ For big-endian vectors only allow vectors of the form { a, a, a..., a }.
+ FIXME: Implement logic for big-endian vectors.  */
+  if (BYTES_BIG_ENDIAN && vector && !const_vec_duplicate_p (op))
+return -1;
+
/* Splat vector constant out into a byte vector.  */
for (i = 0; i < n_elts; i++)
  {

Re: [PATCH][ARM] PR target/71270 fix neon_valid_immediate for big-endian

[Richard Earnshaw (lists)]

On 06/01/17 11:54, Kyrill Tkachov wrote:
> Hi all,
> 
> In this wrong-code issue the RTL tries to load a const_vector:
> (const_vector:V8QI [
> (const_int 1 [0x1])
> (const_int 0 [0])
> (const_int 1 [0x1])
> (const_int 0 [0])
> (const_int 1 [0x1])
> (const_int 0 [0])
> (const_int 1 [0x1])
> (const_int 0 [0])
> ])
> 
> into a NEON register. The logic for that is in neon_valid_immediate
> which does a number of tricks
> to decide what value and of what size to do a VMOV on to load the
> correct vector immediate into the register.
> It goes wrong on big-endian. On both big and little-endian this outputs:
> vmov.i16d18, #0x1
> 
> This is wrong on big-endian. The vector layout has to be such as if
> loaded from memory.
> I've tried various approaches of fixing neon_valid_immediate to generate
> the correct immediate but have been unsuccessful,
> resulting in regressions in various parts of the testsuite or making a
> big mess of the function.
> 
> Given that armeb is not a target of major concern I believe the safest
> route at this stage is to only allow vector constants
> that will obviously work on big-endian, that is the ones that are just a
> single element duplicated in all lanes.
> 
> This patch fixes the execution failures:
> FAIL: gfortran.dg/intrinsic_pack_1.f90
> FAIL: gfortran.dg/c_f_pointer_logical.f03
> FAIL: gcc.dg/torture/pr60183.c
> FAIL: gcc.dg/vect/pr51581-4.c
> 
> on armeb-none-eabi.
> 
> Ok for trunk?
> 
> Thanks,
> Kyrill
> 
> 2017-01-06  Kyrylo Tkachov  
> 
> PR target/71270
> * config/arm/arm.c (neon_valid_immediate): Reject vector constants
> in big-endian mode when they are not a single duplicated value.
> 

Ok, but if you plan to close the PR above, please create a new
'enhancement' PR to fix the missed optimization.

R.

> armeb-vec.patch
> 
> 
> diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
> index 
> 45e0a3cd11fa650f456f7382ffbbcd1c932b95eb..2beee8ebe94eeadd6fb1d5b119e3b474e1ab902a
>  100644
> --- a/gcc/config/arm/arm.c
> +++ b/gcc/config/arm/arm.c
> @@ -11542,6 +11542,12 @@ neon_valid_immediate (rtx op, machine_mode mode, int 
> inverse,
>   return 18;
>  }
>  
> +  /* The tricks done in the code below apply for little-endian vector layout.
> + For big-endian vectors only allow vectors of the form { a, a, a..., a }.
> + FIXME: Implement logic for big-endian vectors.  */
> +  if (BYTES_BIG_ENDIAN && vector && !const_vec_duplicate_p (op))
> +return -1;
> +
>/* Splat vector constant out into a byte vector.  */
>for (i = 0; i < n_elts; i++)
>  {
> 

Re: [PATCH][ARM] PR target/71270 fix neon_valid_immediate for big-endian

[Kyrill Tkachov]

Ping.
https://gcc.gnu.org/ml/gcc-patches/2017-01/msg00381.html

Thanks,
Kyrill

On 06/01/17 11:54, Kyrill Tkachov wrote:

Hi all,

In this wrong-code issue the RTL tries to load a const_vector:
(const_vector:V8QI [
(const_int 1 [0x1])
(const_int 0 [0])
(const_int 1 [0x1])
(const_int 0 [0])
(const_int 1 [0x1])
(const_int 0 [0])
(const_int 1 [0x1])
(const_int 0 [0])
])

into a NEON register. The logic for that is in neon_valid_immediate which does 
a number of tricks
to decide what value and of what size to do a VMOV on to load the correct 
vector immediate into the register.
It goes wrong on big-endian. On both big and little-endian this outputs:
vmov.i16d18, #0x1

This is wrong on big-endian. The vector layout has to be such as if loaded from 
memory.
I've tried various approaches of fixing neon_valid_immediate to generate the 
correct immediate but have been unsuccessful,
resulting in regressions in various parts of the testsuite or making a big mess 
of the function.

Given that armeb is not a target of major concern I believe the safest route at 
this stage is to only allow vector constants
that will obviously work on big-endian, that is the ones that are just a single 
element duplicated in all lanes.

This patch fixes the execution failures:
FAIL: gfortran.dg/intrinsic_pack_1.f90
FAIL: gfortran.dg/c_f_pointer_logical.f03
FAIL: gcc.dg/torture/pr60183.c
FAIL: gcc.dg/vect/pr51581-4.c

on armeb-none-eabi.

Ok for trunk?

Thanks,
Kyrill

2017-01-06  Kyrylo Tkachov  

PR target/71270
* config/arm/arm.c (neon_valid_immediate): Reject vector constants
in big-endian mode when they are not a single duplicated value.

[PATCH][ARM] PR target/71270 fix neon_valid_immediate for big-endian

[Kyrill Tkachov]

Hi all,

In this wrong-code issue the RTL tries to load a const_vector:
(const_vector:V8QI [
(const_int 1 [0x1])
(const_int 0 [0])
(const_int 1 [0x1])
(const_int 0 [0])
(const_int 1 [0x1])
(const_int 0 [0])
(const_int 1 [0x1])
(const_int 0 [0])
])

into a NEON register. The logic for that is in neon_valid_immediate which does 
a number of tricks
to decide what value and of what size to do a VMOV on to load the correct 
vector immediate into the register.
It goes wrong on big-endian. On both big and little-endian this outputs:
vmov.i16d18, #0x1

This is wrong on big-endian. The vector layout has to be such as if loaded from 
memory.
I've tried various approaches of fixing neon_valid_immediate to generate the 
correct immediate but have been unsuccessful,
resulting in regressions in various parts of the testsuite or making a big mess 
of the function.

Given that armeb is not a target of major concern I believe the safest route at 
this stage is to only allow vector constants
that will obviously work on big-endian, that is the ones that are just a single 
element duplicated in all lanes.

This patch fixes the execution failures:
FAIL: gfortran.dg/intrinsic_pack_1.f90
FAIL: gfortran.dg/c_f_pointer_logical.f03
FAIL: gcc.dg/torture/pr60183.c
FAIL: gcc.dg/vect/pr51581-4.c

on armeb-none-eabi.

Ok for trunk?

Thanks,
Kyrill

2017-01-06  Kyrylo Tkachov  

PR target/71270
* config/arm/arm.c (neon_valid_immediate): Reject vector constants
in big-endian mode when they are not a single duplicated value.
diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
index 45e0a3cd11fa650f456f7382ffbbcd1c932b95eb..2beee8ebe94eeadd6fb1d5b119e3b474e1ab902a 100644
--- a/gcc/config/arm/arm.c
+++ b/gcc/config/arm/arm.c
@@ -11542,6 +11542,12 @@ neon_valid_immediate (rtx op, machine_mode mode, int inverse,
 	return 18;
 }
 
+  /* The tricks done in the code below apply for little-endian vector layout.
+ For big-endian vectors only allow vectors of the form { a, a, a..., a }.
+ FIXME: Implement logic for big-endian vectors.  */
+  if (BYTES_BIG_ENDIAN && vector && !const_vec_duplicate_p (op))
+return -1;
+
   /* Splat vector constant out into a byte vector.  */
   for (i = 0; i < n_elts; i++)
 {