Fix wrong code for return of small aggregates on big-endian

Message ID 22150809.kPoQm0lI1j@polaris
State New
Headers show

Commit Message

Eric Botcazou Jan. 9, 2017, 10:43 a.m.
Hi,

this is a regression present on all active branches for big-endian targets 
returning small aggregate types in registers under certain circumstances and 
when optimization is enabled: when the bitfield path of store_field is taken, 
the function ends up calling store_bit_field to store the value.  Now the 
behavior of store_bit_field is awkward when the mode is BLKmode: it always 
takes its value from the lsb up to the word size but expects it left justified 
beyond it (see expmed.c:890 and below) and I missed that when I got rid of the 
stack temporaries that were originally generated in that case.

Of course that's OK for little-endian targets but not for big-endian targets, 
and I have a couple of C++ testcases exposing the issue on SPARC 64-bit and a 
couple of Ada testcases exposing the issue on PowerPC with the SVR4 ABI (the 
Linux ABI is immune since it always returns on the stack); I think they cover 
all the cases in the problematic code.

The attached fix was tested on a bunch of platforms: x86/Linux, x86-64/Linux, 
PowerPC/Linux, PowerPC64/Linux, PowerPC/VxWorks, Aarch64/Linux, SPARC/Solaris 
and SPARC64/Solaris with no regressions.  OK for the mainline? other branches?


2017-01-09  Eric Botcazou  <ebotcazou@adacore.com>

	* expr.c (store_field): In the bitfield case, if the value comes from
	a function call and is of an aggregate type returned in registers, do
	not modify the field mode; extract the value in all cases if the mode
	is BLKmode and the size is not larger than a word.


2017-01-09  Eric Botcazou  <ebotcazou@adacore.com>

	* g++.dg/opt/call2.C: New test.
	* g++.dg/opt/call3.C: Likewise.
	* gnat.dg/array26.adb: New test.
	* gnat.dg/array26_pkg.ad[sb]: New helper.
	* gnat.dg/array27.adb: New test.
	* gnat.dg/array27_pkg.ad[sb]: New helper.
	* gnat.dg/array28.adb: New test.
	* gnat.dg/array28_pkg.ad[sb]: New helper.

-- 
Eric Botcazou

Comments

Richard Biener Jan. 9, 2017, 11:14 a.m. | #1
On Mon, Jan 9, 2017 at 11:43 AM, Eric Botcazou <ebotcazou@adacore.com> wrote:
> Hi,

>

> this is a regression present on all active branches for big-endian targets

> returning small aggregate types in registers under certain circumstances and

> when optimization is enabled: when the bitfield path of store_field is taken,

> the function ends up calling store_bit_field to store the value.  Now the

> behavior of store_bit_field is awkward when the mode is BLKmode: it always

> takes its value from the lsb up to the word size but expects it left justified

> beyond it (see expmed.c:890 and below) and I missed that when I got rid of the

> stack temporaries that were originally generated in that case.

>

> Of course that's OK for little-endian targets but not for big-endian targets,

> and I have a couple of C++ testcases exposing the issue on SPARC 64-bit and a

> couple of Ada testcases exposing the issue on PowerPC with the SVR4 ABI (the

> Linux ABI is immune since it always returns on the stack); I think they cover

> all the cases in the problematic code.

>

> The attached fix was tested on a bunch of platforms: x86/Linux, x86-64/Linux,

> PowerPC/Linux, PowerPC64/Linux, PowerPC/VxWorks, Aarch64/Linux, SPARC/Solaris

> and SPARC64/Solaris with no regressions.  OK for the mainline? other branches?


Ok for trunk and branches after a short burn-in.

Thanks,
Richard.

>

> 2017-01-09  Eric Botcazou  <ebotcazou@adacore.com>

>

>         * expr.c (store_field): In the bitfield case, if the value comes from

>         a function call and is of an aggregate type returned in registers, do

>         not modify the field mode; extract the value in all cases if the mode

>         is BLKmode and the size is not larger than a word.

>

>

> 2017-01-09  Eric Botcazou  <ebotcazou@adacore.com>

>

>         * g++.dg/opt/call2.C: New test.

>         * g++.dg/opt/call3.C: Likewise.

>         * gnat.dg/array26.adb: New test.

>         * gnat.dg/array26_pkg.ad[sb]: New helper.

>         * gnat.dg/array27.adb: New test.

>         * gnat.dg/array27_pkg.ad[sb]: New helper.

>         * gnat.dg/array28.adb: New test.

>         * gnat.dg/array28_pkg.ad[sb]: New helper.

>

> --

> Eric Botcazou
Christophe Lyon Jan. 10, 2017, 8:01 a.m. | #2
On 9 January 2017 at 12:14, Richard Biener <richard.guenther@gmail.com> wrote:
> On Mon, Jan 9, 2017 at 11:43 AM, Eric Botcazou <ebotcazou@adacore.com> wrote:

>> Hi,

>>

>> this is a regression present on all active branches for big-endian targets

>> returning small aggregate types in registers under certain circumstances and

>> when optimization is enabled: when the bitfield path of store_field is taken,

>> the function ends up calling store_bit_field to store the value.  Now the

>> behavior of store_bit_field is awkward when the mode is BLKmode: it always

>> takes its value from the lsb up to the word size but expects it left justified

>> beyond it (see expmed.c:890 and below) and I missed that when I got rid of the

>> stack temporaries that were originally generated in that case.

>>

>> Of course that's OK for little-endian targets but not for big-endian targets,

>> and I have a couple of C++ testcases exposing the issue on SPARC 64-bit and a

>> couple of Ada testcases exposing the issue on PowerPC with the SVR4 ABI (the

>> Linux ABI is immune since it always returns on the stack); I think they cover

>> all the cases in the problematic code.

>>

>> The attached fix was tested on a bunch of platforms: x86/Linux, x86-64/Linux,

>> PowerPC/Linux, PowerPC64/Linux, PowerPC/VxWorks, Aarch64/Linux, SPARC/Solaris

>> and SPARC64/Solaris with no regressions.  OK for the mainline? other branches?

>

> Ok for trunk and branches after a short burn-in.

>


Hi Eric,

I have noticed new failures after this commit (r244249).
g++.dg/opt/call3.C fails at execution on armeb targets
g++.dg/opt/call2.C fails at execution on aarch64_be

Christophe




> Thanks,

> Richard.

>

>>

>> 2017-01-09  Eric Botcazou  <ebotcazou@adacore.com>

>>

>>         * expr.c (store_field): In the bitfield case, if the value comes from

>>         a function call and is of an aggregate type returned in registers, do

>>         not modify the field mode; extract the value in all cases if the mode

>>         is BLKmode and the size is not larger than a word.

>>

>>

>> 2017-01-09  Eric Botcazou  <ebotcazou@adacore.com>

>>

>>         * g++.dg/opt/call2.C: New test.

>>         * g++.dg/opt/call3.C: Likewise.

>>         * gnat.dg/array26.adb: New test.

>>         * gnat.dg/array26_pkg.ad[sb]: New helper.

>>         * gnat.dg/array27.adb: New test.

>>         * gnat.dg/array27_pkg.ad[sb]: New helper.

>>         * gnat.dg/array28.adb: New test.

>>         * gnat.dg/array28_pkg.ad[sb]: New helper.

>>

>> --

>> Eric Botcazou
Eric Botcazou Jan. 10, 2017, 8:58 a.m. | #3
> I have noticed new failures after this commit (r244249).

> g++.dg/opt/call3.C fails at execution on armeb targets

> g++.dg/opt/call2.C fails at execution on aarch64_be


They are new testcases: can you find out whether they pass before the patch?

-- 
Eric Botcazou
Christophe Lyon Jan. 10, 2017, 10:20 a.m. | #4
On 10 January 2017 at 09:58, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> I have noticed new failures after this commit (r244249).

>> g++.dg/opt/call3.C fails at execution on armeb targets

>> g++.dg/opt/call2.C fails at execution on aarch64_be

>

> They are new testcases: can you find out whether they pass before the patch?

>

They pass before the patch (I only checked armeb).

> --

> Eric Botcazou
Eric Botcazou Jan. 10, 2017, 10:26 a.m. | #5
> They pass before the patch (I only checked armeb).


Thanks, I see what's going on, but can you post the configure line of armeb?

-- 
Eric Botcazou
Christophe Lyon Jan. 10, 2017, 11:09 a.m. | #6
On 10 January 2017 at 11:26, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> They pass before the patch (I only checked armeb).

>

> Thanks, I see what's going on, but can you post the configure line of armeb?

>

Sure, it is:
--target=armeb-none-linux-gnueabihf  --with-float=hard --with-mode=arm
--with-cpu=cortex-a9 --with-fpu=neon


> --

> Eric Botcazou
Eric Botcazou Jan. 10, 2017, 12:14 p.m. | #7
> They pass before the patch (I only checked armeb).


I think that's not true for aarch64_be though, since the patch doesn't change 
code generation for this target.  But I'll fix that too.

-- 
Eric Botcazou

Patch hide | download patch | download mbox

Index: expr.c
===================================================================
--- expr.c	(revision 244194)
+++ expr.c	(working copy)
@@ -6888,33 +6888,30 @@  store_field (rtx target, HOST_WIDE_INT b
       if (GET_CODE (temp) == PARALLEL)
 	{
 	  HOST_WIDE_INT size = int_size_in_bytes (TREE_TYPE (exp));
-	  rtx temp_target;
-	  if (mode == BLKmode || mode == VOIDmode)
-	    mode = smallest_mode_for_size (size * BITS_PER_UNIT, MODE_INT);
-	  temp_target = gen_reg_rtx (mode);
+	  machine_mode temp_mode
+	    = smallest_mode_for_size (size * BITS_PER_UNIT, MODE_INT);
+	  rtx temp_target = gen_reg_rtx (temp_mode);
 	  emit_group_store (temp_target, temp, TREE_TYPE (exp), size);
 	  temp = temp_target;
 	}
-      else if (mode == BLKmode)
+
+      /* Handle calls that return BLKmode values in registers.  */
+      else if (mode == BLKmode && REG_P (temp) && TREE_CODE (exp) == CALL_EXPR)
+	{
+	  rtx temp_target = gen_reg_rtx (GET_MODE (temp));
+	  copy_blkmode_from_reg (temp_target, temp, TREE_TYPE (exp));
+	  temp = temp_target;
+	}
+
+      /* The behavior of store_bit_field is awkward when mode is BLKmode:
+	 it always takes its value from the lsb up to the word size but
+	 expects it left justified beyond it.  At this point TEMP is left
+	 justified so extract the value in the former case.  */
+      if (mode == BLKmode && bitsize <= BITS_PER_WORD)
 	{
-	  /* Handle calls that return BLKmode values in registers.  */
-	  if (REG_P (temp) && TREE_CODE (exp) == CALL_EXPR)
-	    {
-	      rtx temp_target = gen_reg_rtx (GET_MODE (temp));
-	      copy_blkmode_from_reg (temp_target, temp, TREE_TYPE (exp));
-	      temp = temp_target;
-	    }
-	  else
-	    {
-	      HOST_WIDE_INT size = int_size_in_bytes (TREE_TYPE (exp));
-	      rtx temp_target;
-	      mode = smallest_mode_for_size (size * BITS_PER_UNIT, MODE_INT);
-	      temp_target = gen_reg_rtx (mode);
-	      temp_target
-	        = extract_bit_field (temp, size * BITS_PER_UNIT, 0, 1,
-				     temp_target, mode, mode, false);
-	      temp = temp_target;
-	    }
+	  machine_mode temp_mode = smallest_mode_for_size (bitsize, MODE_INT);
+	  temp = extract_bit_field (temp, bitsize, 0, 1, NULL_RTX, temp_mode,
+				    temp_mode, false);
 	}
 
       /* Store the value in the bitfield.  */