Re: [PATCH, ARM] stop changing signedness in PROMOTE_MODE

[Richard Earnshaw <Richard dot Earnshaw at foss dot arm dot com>]

On 30/06/15 02:15, Jim Wilson wrote:
> This is my suggested fix for PR 65932, which is a linux kernel
> miscompile with gcc-5.1.
> 
> The problem here is caused by a chain of events.  The first is that
> the relatively new eipa_sra pass creates fake parameters that behave
> slightly differently than normal parameters.  The second is that the
> optimizer creates phi nodes that copy local variables to fake
> parameters and/or vice versa.  The third is that the ouf-of-ssa pass
> assumes that it can emit simple move instructions for these phi nodes.
> And the fourth is that the ARM port has a PROMOTE_MODE macro that
> forces QImode and HImode to unsigned, but a
> TARGET_PROMOTE_FUNCTION_MODE hook that does not.  So signed char and
> short parameters have different in register representations than local
> variables, and require a conversion when copying between them, a
> conversion that the out-of-ssa pass can't easily emit.
> 
> Ultimately, I think this is a problem in the arm backend.  It should
> not have a PROMOTE_MODE macro that is changing the sign of char and
> short local variables.  I also think that we should merge the
> PROMOTE_MODE macro with the TARGET_PROMOTE_FUNCTION_MODE hook to
> prevent this from happening again.
> 

The documentation for PROMOTE_MODE says:

For most machines, the macro definition does not change @var{unsignedp}.
However, some machines, have instructions that preferentially handle
either signed or unsigned quantities of certain modes.  For example, on
the DEC Alpha, 32-bit loads from memory and 32-bit add instructions
sign-extend the result to 64 bits.  On such machines, set
@var{unsignedp} according to which kind of extension is more efficient.

So it seems to me that the ARM backend is only doing what the
documentation says should work.

> I see four general problems with the current ARM PROMOTE_MODE definition.
> 1) Unsigned char is only faster for armv5 and earlier, before the sxtb
> instruction was added.  It is a lose for armv6 and later.

Not quite, ARM state still has more flexible addressing modes for
unsigned byte loads than for signed byte loads.  It's even worse with
thumb1 where some signed loads have no single-register addressing mode
(ie you have to copy zero into another register to use as an index
before doing the load).


R.

> 2) Unsigned short was only faster for targets that don't support
> unaligned accesses.  Support for these targets was removed a while
> ago, and this PROMODE_MODE hunk should have been removed at the same
> time.  It was accidentally left behind.
> 3) TARGET_PROMOTE_FUNCTION_MODE used to be a boolean hook, when it was
> converted to a function, the PROMOTE_MODE code was copied without the
> UNSIGNEDP changes.  Thus it is only an accident that
> TARGET_PROMOTE_FUNCTION_MODE and PROMOTE_MODE disagree.  Changing
> TARGET_PROMOTE_FUNCTION_MODE is an ABI change, so only PROMOTE_MODE
> changes to resolve the difference are safe.
> 4) There is a general principle that you should only change signedness
> in PROMOTE_MODE if the hardware forces it, as otherwise this results
> in extra conversion instructions that make code slower.  The mips64
> hardware for instance requires that 32-bit values be sign-extended
> regardless of type, and instructions may trap if this is not true.
> However, it has a set of 32-bit instructions that operate on these
> values, and hence no conversions are required.  There is no similar
> case on ARM. Thus the conversions are unnecessary and unwise.  This
> can be seen in the testcases where gcc emits both a zero-extend and a
> sign-extend inside a loop, as the sign-extend is required for a
> compare, and the zero-extend is required by PROMOTE_MODE.
> 
> My change was tested with an arm bootstrap, make check, and SPEC
> CPU2000 run.  The original poster verified that this gives a linux
> kernel that boots correctly.
> 
> The PRMOTE_MODE change causes 3 testsuite testcases to fail.  These
> are tests to verify that smulbb and/or smlabb are generated.
> Eliminating the unnecessary sign conversions causes us to get better
> code that doesn't include the smulbb and smlabb instructions.  I had
> to modify the testcases to get them to emit the desired instructions.
> With the testcase changes there are no additional testsuite failures,
> though I'm concerned that these testcases with the changes may be
> fragile, and future changes may break them again.
> 
> If there are ARM parts where smulbb/smlabb are faster than mul/mla,
> then maybe we should try to add new patterns to get the instructions
> emitted again for the unmodified testcases.
> 
> Jim
> 
> 
> pr65932-3.patch
> 
> 
> gcc/
> 2015-06-29  Jim Wilson  <jim.wilson@linaro.org>
> 
> 	PR target/65932
> 	* config/arm/arm.h (PROMOTE_MODE): Don't set UNSIGNEDP for QImode and
> 	HImode.
> 
> gcc/testsuite/
> 2015-06-29  Jim Wilson  <jim.wilson@linaro.org>
> 
> 	PR target/65932
> 	* gcc.target/arm/wmul-1.c (mac): Change a and b to int pointers.  Cast
> 	multiply operands to short.
> 	* gcc.target/arm/wmul-2.c (vec_mpy): Cast multiply result to short.
> 	* gcc.target/arm/wmul-3.c (mac): Cast multiply results to short.
> 
> Index: config/arm/arm.h
> ===================================================================
> --- config/arm/arm.h	(revision 224672)
> +++ config/arm/arm.h	(working copy)
> @@ -523,16 +523,10 @@ extern int arm_arch_crc;
>     type, but kept valid in the wider mode.  The signedness of the
>     extension may differ from that of the type.  */
>  
> -/* It is far faster to zero extend chars than to sign extend them */
> -
>  #define PROMOTE_MODE(MODE, UNSIGNEDP, TYPE)	\
>    if (GET_MODE_CLASS (MODE) == MODE_INT		\
>        && GET_MODE_SIZE (MODE) < 4)      	\
>      {						\
> -      if (MODE == QImode)			\
> -	UNSIGNEDP = 1;				\
> -      else if (MODE == HImode)			\
> -	UNSIGNEDP = 1;				\
>        (MODE) = SImode;				\
>      }
>  
> Index: testsuite/gcc.target/arm/wmul-1.c
> ===================================================================
> --- testsuite/gcc.target/arm/wmul-1.c	(revision 224672)
> +++ testsuite/gcc.target/arm/wmul-1.c	(working copy)
> @@ -2,14 +2,14 @@
>  /* { dg-require-effective-target arm_dsp } */
>  /* { dg-options "-O1 -fexpensive-optimizations" } */
>  
> -int mac(const short *a, const short *b, int sqr, int *sum)
> +int mac(const int *a, const int *b, int sqr, int *sum)
>  {
>    int i;
>    int dotp = *sum;
>  
>    for (i = 0; i < 150; i++) {
> -    dotp += b[i] * a[i];
> -    sqr += b[i] * b[i];
> +    dotp += (short)b[i] * (short)a[i];
> +    sqr += (short)b[i] * (short)b[i];
>    }
>  
>    *sum = dotp;
> Index: testsuite/gcc.target/arm/wmul-2.c
> ===================================================================
> --- testsuite/gcc.target/arm/wmul-2.c	(revision 224672)
> +++ testsuite/gcc.target/arm/wmul-2.c	(working copy)
> @@ -7,7 +7,7 @@ void vec_mpy(int y[], const short x[], s
>   int i;
>  
>   for (i = 0; i < 150; i++)
> -   y[i] += ((scaler * x[i]) >> 31);
> +   y[i] += ((short)(scaler * x[i]) >> 31);
>  }
>  
>  /* { dg-final { scan-assembler-times "smulbb" 1 } } */
> Index: testsuite/gcc.target/arm/wmul-3.c
> ===================================================================
> --- testsuite/gcc.target/arm/wmul-3.c	(revision 224672)
> +++ testsuite/gcc.target/arm/wmul-3.c	(working copy)
> @@ -8,8 +8,8 @@ int mac(const short *a, const short *b,
>    int dotp = *sum;
>  
>    for (i = 0; i < 150; i++) {
> -    dotp -= b[i] * a[i];
> -    sqr -= b[i] * b[i];
> +    dotp -= (short)(b[i] * a[i]);
> +    sqr -= (short)(b[i] * b[i]);
>    }
>  
>    *sum = dotp;
>