Re: [PATCH][arm] PR target/82518: Return false in ARRAY_MODE_SUPPORTED_P for BYTES_BIG_ENDIAN
On 25/04/18 18:31, Maxim Kuvyrkov wrote: On Mar 20, 2018, at 8:11 PM, Kyrill Tkachov wrote: Hi all, This PR shows that we get the load/store_lanes logic wrong for arm big-endian. It is tricky to get right. Aarch64 does it by adding the appropriate lane-swapping operations during expansion. I'd like to do the same on arm eventually, but we'd need to port and validate the VTBL-generating code and add it to all the right places and I'm not comfortable enough doing it for GCC 8, but I am keen in getting the wrong-code fixed. As I say in the PR, vectorisation on armeb is already severely restricted (we disable many patterns on BYTES_BIG_ENDIAN) and the load/store_lanes patterns really were not working properly at all, so disabling them is not a radical approach. The way to do that is to return false in ARRAY_MODE_SUPPORTED_P for BYTES_BIG_ENDIAN. Bootstrapped and tested on arm-none-linux-gnueabihf. Also tested on armeb-none-eabi. Committing to trunk. ... --- a/gcc/testsuite/lib/target-supports.exp +++ b/gcc/testsuite/lib/target-supports.exp @@ -6609,7 +6609,8 @@ proc check_effective_target_vect_load_lanes { } { verbose "check_effective_target_vect_load_lanes: using cached result" 2 } else { set et_vect_load_lanes 0 - if { ([istarget arm*-*-*] && [check_effective_target_arm_neon_ok]) + # We don't support load_lanes correctly on big-endian arm. + if { ([istarget arm-*-*] && [check_effective_target_arm_neon_ok]) || [istarget aarch64*-*-*] } { set et_vect_load_lanes 1 } Hi Kyrill, This part makes armv8l-linux-gnueabihf target fail a few of slp-perm-* tests. Using [check_effective_target_arm_little_endian] should fix this. Would you please fix this on master and gcc-7-branch? Hi Maxim, Thanks for catching this. This patch fixes the failures (I've reproduced them on a armv8l-linux-gnueabihf target). Committing to trunk. Richi, Jakub, is this ok to commit to the GCC 8 branch at this time? Thanks, Kyrill 2018-04-27 Kyrylo Tkachov * lib/target-supports.exp (check_effective_target_vect_load_lanes): Use check_effective_target_arm_little_endian. diff --git a/gcc/testsuite/lib/target-supports.exp b/gcc/testsuite/lib/target-supports.exp index 00f988cc756b4f5898b9c6290722d1353240b3a3..6d497fbcf8033cae06f54e0601f14e4affe29923 100644 --- a/gcc/testsuite/lib/target-supports.exp +++ b/gcc/testsuite/lib/target-supports.exp @@ -6588,7 +6588,7 @@ proc check_effective_target_vect_load_lanes { } { } else { set et_vect_load_lanes 0 # We don't support load_lanes correctly on big-endian arm. - if { ([istarget arm-*-*] && [check_effective_target_arm_neon_ok]) + if { ([check_effective_target_arm_little_endian] && [check_effective_target_arm_neon_ok]) || [istarget aarch64*-*-*] } { set et_vect_load_lanes 1 }
Re: [PATCH][arm] PR target/82518: Return false in ARRAY_MODE_SUPPORTED_P for BYTES_BIG_ENDIAN
> On Mar 20, 2018, at 8:11 PM, Kyrill Tkachov > wrote: > > Hi all, > > This PR shows that we get the load/store_lanes logic wrong for arm big-endian. > It is tricky to get right. Aarch64 does it by adding the appropriate > lane-swapping > operations during expansion. > > I'd like to do the same on arm eventually, but we'd need to port and validate > the VTBL-generating > code and add it to all the right places and I'm not comfortable enough doing > it for GCC 8, but I am keen > in getting the wrong-code fixed. > As I say in the PR, vectorisation on armeb is already severely restricted (we > disable many patterns on BYTES_BIG_ENDIAN) > and the load/store_lanes patterns really were not working properly at all, so > disabling them is not > a radical approach. > > The way to do that is to return false in ARRAY_MODE_SUPPORTED_P for > BYTES_BIG_ENDIAN. > > Bootstrapped and tested on arm-none-linux-gnueabihf. > Also tested on armeb-none-eabi. > > Committing to trunk. ... > --- a/gcc/testsuite/lib/target-supports.exp > +++ b/gcc/testsuite/lib/target-supports.exp > @@ -6609,7 +6609,8 @@ proc check_effective_target_vect_load_lanes { } { > verbose "check_effective_target_vect_load_lanes: using cached result" 2 > } else { > set et_vect_load_lanes 0 > - if { ([istarget arm*-*-*] && [check_effective_target_arm_neon_ok]) > + # We don't support load_lanes correctly on big-endian arm. > + if { ([istarget arm-*-*] && [check_effective_target_arm_neon_ok]) >|| [istarget aarch64*-*-*] } { > set et_vect_load_lanes 1 > } > Hi Kyrill, This part makes armv8l-linux-gnueabihf target fail a few of slp-perm-* tests. Using [check_effective_target_arm_little_endian] should fix this. Would you please fix this on master and gcc-7-branch? Thanks! -- Maxim Kuvyrkov www.linaro.org
Re: [PATCH][arm] PR target/82518: Return false in ARRAY_MODE_SUPPORTED_P for BYTES_BIG_ENDIAN
On 21/03/18 09:07, Christophe Lyon wrote: On 20 March 2018 at 18:11, Kyrill Tkachov wrote: Hi all, This PR shows that we get the load/store_lanes logic wrong for arm big-endian. It is tricky to get right. Aarch64 does it by adding the appropriate lane-swapping operations during expansion. I'd like to do the same on arm eventually, but we'd need to port and validate the VTBL-generating code and add it to all the right places and I'm not comfortable enough doing it for GCC 8, but I am keen in getting the wrong-code fixed. As I say in the PR, vectorisation on armeb is already severely restricted (we disable many patterns on BYTES_BIG_ENDIAN) and the load/store_lanes patterns really were not working properly at all, so disabling them is not a radical approach. The way to do that is to return false in ARRAY_MODE_SUPPORTED_P for BYTES_BIG_ENDIAN. Bootstrapped and tested on arm-none-linux-gnueabihf. Also tested on armeb-none-eabi. Committing to trunk. Thanks, Kyrill 2018-03-20 Kyrylo Tkachov PR target/82518 * config/arm/arm.c (arm_array_mode_supported_p): Return false for BYTES_BIG_ENDIAN. 2018-03-20 Kyrylo Tkachov PR target/82518 * lib/target-supports.exp (check_effective_target_vect_load_lanes): Disable for armeb targets. * gcc.target/arm/pr82518.c: New test. Hi Kyrill, I think you need: Index: pr82518.c === --- pr82518.c (revision 258705) +++ pr82518.c (working copy) @@ -1,5 +1,5 @@ /* { dg-do run } */ -/* { dg-require-effective-target arm_neon_ok } */ +/* { dg-require-effective-target arm_neon_hw } */ /* { dg-additional-options "-O3 -fno-inline -std=gnu99" } */ /* { dg-add-options arm_neon } */ (The test fails to run for me when using qemu --cpu arm926) OK? Yes, you're right. Thanks Christophe. Kyrill Christophe
Re: [PATCH][arm] PR target/82518: Return false in ARRAY_MODE_SUPPORTED_P for BYTES_BIG_ENDIAN
On 20 March 2018 at 18:11, Kyrill Tkachov wrote: > Hi all, > > This PR shows that we get the load/store_lanes logic wrong for arm > big-endian. > It is tricky to get right. Aarch64 does it by adding the appropriate > lane-swapping > operations during expansion. > > I'd like to do the same on arm eventually, but we'd need to port and > validate the VTBL-generating > code and add it to all the right places and I'm not comfortable enough doing > it for GCC 8, but I am keen > in getting the wrong-code fixed. > As I say in the PR, vectorisation on armeb is already severely restricted > (we disable many patterns on BYTES_BIG_ENDIAN) > and the load/store_lanes patterns really were not working properly at all, > so disabling them is not > a radical approach. > > The way to do that is to return false in ARRAY_MODE_SUPPORTED_P for > BYTES_BIG_ENDIAN. > > Bootstrapped and tested on arm-none-linux-gnueabihf. > Also tested on armeb-none-eabi. > > Committing to trunk. > Thanks, > Kyrill > > 2018-03-20 Kyrylo Tkachov > > PR target/82518 > * config/arm/arm.c (arm_array_mode_supported_p): Return false for > BYTES_BIG_ENDIAN. > > 2018-03-20 Kyrylo Tkachov > > PR target/82518 > * lib/target-supports.exp (check_effective_target_vect_load_lanes): > Disable for armeb targets. > * gcc.target/arm/pr82518.c: New test. Hi Kyrill, I think you need: Index: pr82518.c === --- pr82518.c (revision 258705) +++ pr82518.c (working copy) @@ -1,5 +1,5 @@ /* { dg-do run } */ -/* { dg-require-effective-target arm_neon_ok } */ +/* { dg-require-effective-target arm_neon_hw } */ /* { dg-additional-options "-O3 -fno-inline -std=gnu99" } */ /* { dg-add-options arm_neon } */ (The test fails to run for me when using qemu --cpu arm926) OK? Christophe
[PATCH][arm] PR target/82518: Return false in ARRAY_MODE_SUPPORTED_P for BYTES_BIG_ENDIAN
Hi all, This PR shows that we get the load/store_lanes logic wrong for arm big-endian. It is tricky to get right. Aarch64 does it by adding the appropriate lane-swapping operations during expansion. I'd like to do the same on arm eventually, but we'd need to port and validate the VTBL-generating code and add it to all the right places and I'm not comfortable enough doing it for GCC 8, but I am keen in getting the wrong-code fixed. As I say in the PR, vectorisation on armeb is already severely restricted (we disable many patterns on BYTES_BIG_ENDIAN) and the load/store_lanes patterns really were not working properly at all, so disabling them is not a radical approach. The way to do that is to return false in ARRAY_MODE_SUPPORTED_P for BYTES_BIG_ENDIAN. Bootstrapped and tested on arm-none-linux-gnueabihf. Also tested on armeb-none-eabi. Committing to trunk. Thanks, Kyrill 2018-03-20 Kyrylo Tkachov PR target/82518 * config/arm/arm.c (arm_array_mode_supported_p): Return false for BYTES_BIG_ENDIAN. 2018-03-20 Kyrylo Tkachov PR target/82518 * lib/target-supports.exp (check_effective_target_vect_load_lanes): Disable for armeb targets. * gcc.target/arm/pr82518.c: New test. commit 861f9881d3af7e6f94e9b008e88aa7ba7c44a4df Author: Kyrylo Tkachov Date: Tue Feb 20 15:14:01 2018 + [arm] PR target/82518: Return false in ARRAY_MODE_SUPPORTED_P for BYTES_BIG_ENDIAN diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c index 73e9fc1..09deb60 100644 --- a/gcc/config/arm/arm.c +++ b/gcc/config/arm/arm.c @@ -27151,7 +27151,10 @@ static bool arm_array_mode_supported_p (machine_mode mode, unsigned HOST_WIDE_INT nelems) { - if (TARGET_NEON + /* We don't want to enable interleaved loads and stores for BYTES_BIG_ENDIAN + for now, as the lane-swapping logic needs to be extended in the expanders. + See PR target/82518. */ + if (TARGET_NEON && !BYTES_BIG_ENDIAN && (VALID_NEON_DREG_MODE (mode) || VALID_NEON_QREG_MODE (mode)) && (nelems >= 2 && nelems <= 4)) return true; diff --git a/gcc/testsuite/gcc.target/arm/pr82518.c b/gcc/testsuite/gcc.target/arm/pr82518.c new file mode 100644 index 000..c3e45b8 --- /dev/null +++ b/gcc/testsuite/gcc.target/arm/pr82518.c @@ -0,0 +1,29 @@ +/* { dg-do run } */ +/* { dg-require-effective-target arm_neon_ok } */ +/* { dg-additional-options "-O3 -fno-inline -std=gnu99" } */ +/* { dg-add-options arm_neon } */ + +typedef struct { int x, y; } X; + +void f4(X *p, int n) +{ + for (int i = 0; i < n; i++) + { p[i].x = i; +p[i].y = i + 1; + } +} + +__attribute ((aligned (16))) X arr[100]; + +int main(void) +{ + volatile int fail = 0; + f4 (arr, 100); + for (int i = 0; i < 100; i++) +if (arr[i].y != i+1 || arr[i].x != i) + fail = 1; + if (fail) + __builtin_abort (); + + return 0; +} diff --git a/gcc/testsuite/lib/target-supports.exp b/gcc/testsuite/lib/target-supports.exp index 640f76e..5ff5ec6 100644 --- a/gcc/testsuite/lib/target-supports.exp +++ b/gcc/testsuite/lib/target-supports.exp @@ -6609,7 +6609,8 @@ proc check_effective_target_vect_load_lanes { } { verbose "check_effective_target_vect_load_lanes: using cached result" 2 } else { set et_vect_load_lanes 0 - if { ([istarget arm*-*-*] && [check_effective_target_arm_neon_ok]) + # We don't support load_lanes correctly on big-endian arm. + if { ([istarget arm-*-*] && [check_effective_target_arm_neon_ok]) || [istarget aarch64*-*-*] } { set et_vect_load_lanes 1 }