Re: [PATCH, rs6000] Fix PR79268

[Segher Boessenkool <segher at kernel dot crashing dot org>]

Hi Bill,

On Sun, Jan 29, 2017 at 04:17:03PM -0600, Bill Schmidt wrote:
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79268 reports that the
> vec_xl and vec_xst intrinsics are being compiled to incorrectly use
> element-reversing loads and stores in some cases, and not accepting
> pointers to vector pixel as a legal parameter.  This arose because
> vec_xl and vec_xst were incorrectly redefined in altivec.h to be
> equivalent to these instructions; the intent was to use them to
> implement vec_xl_be and vec_xst_be instead.  I.e., I goofed...

I missed it as well :-(

> This affects GCC 6 and GCC 7.  This patch fixes the critical issue by
> restoring vec_xl and vec_xst to their former definitions, and adding a
> test case to ensure that vector pixel is covered by them.  It also
> deletes the four test cases that were added as part of the errant patch.
> This is a simple patch that is intended for application to trunk and
> backport to GCC 6.
> 
> A subsequent patch will address defining vec_xl_be and vec_xst_be in
> terms of the element-reversing memory accesses, which will require more
> extensive changes.  This may or may not be deemed worth backporting, but
> I think that decision should be made independently.

Yeah.

> This first patch will be provided to the various distros to fix and/or
> prevent build problems using GCC 6 on packages that use vec_xl.  At
> least one such package is known to exist.
> 
> Bootstrapped and tested on powerpc64le-unknown-linux-gnu with no
> regressions.  Regstrap is in process for the GCC 6 version of the patch,
> which applies cleanly with offsets.  Is this ok for trunk, and shortly
> thereafter for GCC 6?

Okay for both.  A question and a pedant remark about the new testcase.


> --- gcc/testsuite/gcc.target/powerpc/pr79268.c	(revision 0)
> +++ gcc/testsuite/gcc.target/powerpc/pr79268.c	(working copy)
> @@ -0,0 +1,18 @@
> +/* { dg-do compile { target { powerpc64le-*-* } } } */

Is that correct?  Shouldn't it be target powerpc*-*-* and use -m64 -mlittle
-mabi=elfv2 ?  Or use some LE selector, LP64, etc.

Target doesn't mean very much.

Does it not run fine on BE / elfv1 anyway?

> +/* { dg-skip-if "do not override -mcpu" { powerpc*-*-* } { "-mcpu=*" } { "-mcpu=power8" } } */
> +/* { dg-options "-mcpu=power8 -O3 " } */

Stray space before the closing quote...  Harmless of course.

Thanks,


Segher


> 2017-01-29  Bill Schmidt  <wschmidt@linux.vnet.ibm.com>
> 
> 	PR target/79268
> 	* config/rs6000/altivec.h (vec_xl): Revise #define.
> 	(vec_xst): Likewise.
> 
> [gcc/testsuite]
> 
> 2017-01-29  Bill Schmidt  <wschmidt@linux.vnet.ibm.com>
> 
> 	PR target/79268
> 	* gcc.target/powerpc/pr79268.c: New file.
> 	* gcc.target/powerpc/vsx-elemrev-1.c: Delete file.
> 	* gcc.target/powerpc/vsx-elemrev-2.c: Likewise.
> 	* gcc.target/powerpc/vsx-elemrev-3.c: Likewise.
> 	* gcc.target/powerpc/vsx-elemrev-4.c: Likewise.