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: [PATCH, ARM] Backport fix for PR59593 (minipool of small values on big endian targets)


Ping?

> -----Original Message-----
> From: gcc-patches-owner@gcc.gnu.org [mailto:gcc-patches-
> owner@gcc.gnu.org] On Behalf Of Thomas Preud'homme
> Sent: Tuesday, January 20, 2015 1:06 PM
> To: gcc-patches@gcc.gnu.org; Richard Earnshaw; Ramana Radhakrishnan;
> Marcus Shawcroft
> Subject: [PATCH, ARM] Backport fix for PR59593 (minipool of small values
> on big endian targets)
> 
> Currently on GCC 4.8 and 4.9, constant pool entries for QImode, HImode
> and SImode values are filled as 32-bit quantities. This works fine for little
> endian system but gives some incorrect results for big endian system
> when the value is accessed with a mode smaller than 32-bit in size.
> Suppose the case of the value 0x1234 that is accessed as an HImode
> value. It would be output as 0x0 0x0 0x12 0x34 in a constant pool entry
> and the 16-bit load that would be done would lead to the value 0x0 in
> register.
> 
> This patch makes the consttable_1 and consttable_2 pattern available to
> ARM as well so that values are output according to their mode.
> 
> This is a backport of commit r218118.
> 
> *** gcc/ChangeLog ***
> 
> 2015-01-14  Thomas Preud'homme  <thomas.preudhomme@arm.com>
> 
>     Backport from mainline
>     2014-11-27 Thomas Preud'homme <thomas.preudhomme@arm.com>
> 
>     PR target/59593
>     * config/arm/arm.c (dump_minipool): dispatch to consttable pattern
>     based on mode size.
>     * config/arm/arm.md (consttable_1): Make it TARGET_EITHER.
>     (consttable_2): Make it TARGET_EITHER and move HFmode handling
> from
>     consttable_4 to it.
>     (consttable_4): Move HFmode handling to consttable_2 pattern.
> 
> 
> *** gcc/testsuite/ChangeLog ***
> 
> 2015-01-14  Thomas Preud'homme  <thomas.preudhomme@arm.com>
> 
>     Backport from mainline
>     2014-11-27 Thomas Preud'homme <thomas.preudhomme@arm.com>
> 
>     PR target/59593
>     * gcc.target/arm/constant-pool.c: New test.
> 
> 
> diff --git a/gcc/ChangeLog b/gcc/ChangeLog
> index 85372e5..eeaece8 100644
> --- a/gcc/ChangeLog
> +++ b/gcc/ChangeLog
> @@ -1,3 +1,16 @@
> +2015-01-14  Thomas Preud'homme  <thomas.preudhomme@arm.com>
> +
> +	Backport from mainline
> +	2014-11-27  Thomas Preud'homme
> <thomas.preudhomme@arm.com>
> +
> +	PR target/59593
> +	* config/arm/arm.c (dump_minipool): dispatch to consttable
> pattern
> +	based on mode size.
> +	* config/arm/arm.md (consttable_1): Make it TARGET_EITHER.
> +	(consttable_2): Make it TARGET_EITHER and move HFmode
> handling from
> +	consttable_4 to it.
> +	(consttable_4): Move HFmode handling to consttable_2 pattern.
> +
>  2015-01-14  Marek Polacek  <polacek@redhat.com>
> 
>  	Backport from mainline
> diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
> index 5e2571c..67ef80b 100644
> --- a/gcc/config/arm/arm.c
> +++ b/gcc/config/arm/arm.c
> @@ -16274,7 +16274,7 @@ dump_minipool (rtx scan)
>  	      fputc ('\n', dump_file);
>  	    }
> 
> -	  switch (mp->fix_size)
> +	  switch (GET_MODE_SIZE (mp->mode))
>  	    {
>  #ifdef HAVE_consttable_1
>  	    case 1:
> diff --git a/gcc/config/arm/arm.md b/gcc/config/arm/arm.md
> index 8119387..93b25e9 100644
> --- a/gcc/config/arm/arm.md
> +++ b/gcc/config/arm/arm.md
> @@ -12224,7 +12224,7 @@
> 
>  (define_insn "consttable_1"
>    [(unspec_volatile [(match_operand 0 "" "")] VUNSPEC_POOL_1)]
> -  "TARGET_THUMB1"
> +  "TARGET_EITHER"
>    "*
>    making_const_table = TRUE;
>    assemble_integer (operands[0], 1, BITS_PER_WORD, 1);
> @@ -12237,14 +12237,23 @@
> 
>  (define_insn "consttable_2"
>    [(unspec_volatile [(match_operand 0 "" "")] VUNSPEC_POOL_2)]
> -  "TARGET_THUMB1"
> +  "TARGET_EITHER"
>    "*
> -  making_const_table = TRUE;
> -  gcc_assert (GET_MODE_CLASS (GET_MODE (operands[0])) !=
> MODE_FLOAT);
> -  assemble_integer (operands[0], 2, BITS_PER_WORD, 1);
> -  assemble_zeros (2);
> -  return \"\";
> -  "
> +  {
> +    rtx x = operands[0];
> +    making_const_table = TRUE;
> +    switch (GET_MODE_CLASS (GET_MODE (x)))
> +      {
> +      case MODE_FLOAT:
> +	arm_emit_fp16_const (x);
> +	break;
> +      default:
> +	assemble_integer (operands[0], 2, BITS_PER_WORD, 1);
> +	assemble_zeros (2);
> +	break;
> +      }
> +    return \"\";
> +  }"
>    [(set_attr "length" "4")
>     (set_attr "type" "no_insn")]
>  )
> @@ -12259,15 +12268,12 @@
>      switch (GET_MODE_CLASS (GET_MODE (x)))
>        {
>        case MODE_FLOAT:
> - 	if (GET_MODE (x) == HFmode)
> - 	  arm_emit_fp16_const (x);
> - 	else
> - 	  {
> - 	    REAL_VALUE_TYPE r;
> - 	    REAL_VALUE_FROM_CONST_DOUBLE (r, x);
> - 	    assemble_real (r, GET_MODE (x), BITS_PER_WORD);
> - 	  }
> - 	break;
> +	{
> +	  REAL_VALUE_TYPE r;
> +	  REAL_VALUE_FROM_CONST_DOUBLE (r, x);
> +	  assemble_real (r, GET_MODE (x), BITS_PER_WORD);
> +	  break;
> +	}
>        default:
>  	/* XXX: Sometimes gcc does something really dumb and ends up
> with
>  	   a HIGH in a constant pool entry, usually because it's trying to
> diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog
> index 222716d..49d1a73 100644
> --- a/gcc/testsuite/ChangeLog
> +++ b/gcc/testsuite/ChangeLog
> @@ -1,3 +1,11 @@
> +2015-01-14  Thomas Preud'homme  <thomas.preudhomme@arm.com>
> +
> +	Backport from mainline
> +	2014-11-27  Thomas Preud'homme
> <thomas.preudhomme@arm.com>
> +
> +	PR target/59593
> +	* gcc.target/arm/constant-pool.c: New test.
> +
>  2015-01-14  Marek Polacek  <polacek@redhat.com>
> 
>  	Backport from mainline
> diff --git a/gcc/testsuite/gcc.target/arm/constant-pool.c
> b/gcc/testsuite/gcc.target/arm/constant-pool.c
> new file mode 100644
> index 0000000..8427dfb
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/arm/constant-pool.c
> @@ -0,0 +1,27 @@
> +/* { dg-do run } */
> +/* { dg-options "-O1" } */
> +
> +unsigned short v = 0x5678;
> +int i;
> +int j = 0;
> +int *ptr = &j;
> +
> +int
> +func (void)
> +{
> +  for (i = 0; i < 1; ++i)
> +    {
> +      *ptr = -1;
> +      v = 0x1234;
> +    }
> +  return v;
> +}
> +
> +int
> +main (void)
> +{
> +  func ();
> +  if (v != 0x1234)
> +    __builtin_abort ();
> +  return 0;
> +}
> 
> 
> Is this ok for 4.8 and 4.9 branches?
> 
> Best regards,
> 
> Thomas
> 
> 
> 





Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]