| Message ID | 22150809.kPoQm0lI1j@polaris |
|---|---|
| State | New |
| Headers | show |
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
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
> 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?
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
> 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?
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
> 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.
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. */