[3/8] Make more use of force_subreg

This patch makes target-independent code use force_subreg instead
of simplify_gen_subreg in some places.  The criteria were:

(1) The code is obviously specific to expand (where new pseudos
    can be created), or at least would be invalid to call when
    !can_create_pseudo_p () and temporaries are needed.

(2) The value is obviously an rvalue rather than an lvalue.

(3) The offset wasn't a simple lowpart or highpart calculation;
    a later patch will deal with those.

Doing this should reduce the likelihood of bugs like PR115464
occuring in other situations.

gcc/
	* expmed.cc (store_bit_field_using_insv): Use force_subreg
	instead of simplify_gen_subreg.
	(store_bit_field_1): Likewise.
	(extract_bit_field_as_subreg): Likewise.
	(extract_integral_bit_field): Likewise.
	(emit_store_flag_1): Likewise.
	* expr.cc (convert_move): Likewise.
	(convert_modes): Likewise.
	(emit_group_load_1): Likewise.
	(emit_group_store): Likewise.
	(expand_assignment): Likewise.
---
 gcc/expmed.cc | 22 ++++++++--------------
 gcc/expr.cc   | 27 ++++++++++++---------------
 2 files changed, 20 insertions(+), 29 deletions(-)

On 6/17/24 3:53 AM, Richard Sandiford wrote:
> This patch makes target-independent code use force_subreg instead
> of simplify_gen_subreg in some places.  The criteria were:
> 
> (1) The code is obviously specific to expand (where new pseudos
>      can be created), or at least would be invalid to call when
>      !can_create_pseudo_p () and temporaries are needed.
> 
> (2) The value is obviously an rvalue rather than an lvalue.
> 
> (3) The offset wasn't a simple lowpart or highpart calculation;
>      a later patch will deal with those.
> 
> Doing this should reduce the likelihood of bugs like PR115464
> occuring in other situations.
> 
> gcc/
> 	* expmed.cc (store_bit_field_using_insv): Use force_subreg
> 	instead of simplify_gen_subreg.
> 	(store_bit_field_1): Likewise.
> 	(extract_bit_field_as_subreg): Likewise.
> 	(extract_integral_bit_field): Likewise.
> 	(emit_store_flag_1): Likewise.
> 	* expr.cc (convert_move): Likewise.
> 	(convert_modes): Likewise.
> 	(emit_group_load_1): Likewise.
> 	(emit_group_store): Likewise.
> 	(expand_assignment): Likewise.
[ ... ]

So this has triggered a failure on ft32-elf with this testcase 
(simplified from the testsuite):

typedef _Bool bool;
const bool false = 0;
const bool true = 1;

struct RenderBox
{
   bool m_positioned : 1;
};

typedef struct RenderBox RenderBox;


void RenderBox_setStyle(RenderBox *thisin)
{
   RenderBox *this = thisin;
   bool ltrue = true;
   this->m_positioned = ltrue;

}



Before this change we generated this:

> (insn 13 12 14 (set (reg:QI 47)
>         (mem/c:QI (plus:SI (reg/f:SI 37 virtual-stack-vars)
>                 (const_int -5 [0xfffffffffffffffb])) [1 ltrue+0 S1 A8])) "j.c":17:22 -1
>      (nil))
> 
> (insn 14 13 15 (parallel [
>             (set (zero_extract:SI (subreg:SI (reg:QI 46) 0)
>                     (const_int 1 [0x1])
>                     (const_int 0 [0]))
>                 (subreg:SI (reg:QI 47) 0))
>             (clobber (scratch:SI))
>         ]) "j.c":17:22 -1
>      (nil))


Afterwards we generate:

> (insn 13 12 14 2 (parallel [
>             (set (zero_extract:SI (subreg:SI (reg:QI 46) 0)
>                     (const_int 1 [0x1])
>                     (const_int 0 [0]))
>                 (subreg:SI (mem/c:QI (plus:SI (reg/f:SI 37 virtual-stack-vars)
>                             (const_int -5 [0xfffffffffffffffb])) [1 ltrue+0 S1 A8]) 0))
>             (clobber (scratch:SI))
>         ]) "j.c":17:22 -1
>      (nil))

Note the (subreg (mem (...)).  Probably not desirable in general, but 
also note the virtual-stack-vars in the memory address.  The code to 
instantiate virtual registers doesn't handle (subreg (mem)), so we never 
convert that to an FP based address and we eventually fault.

Should be visible with ft32-elf cross compiler.  No options needed.

Jeff

On Fri, Jun 21, 2024 at 1:11 PM Jeff Law <jeffreyalaw@gmail.com> wrote:
>
>
>
> On 6/17/24 3:53 AM, Richard Sandiford wrote:
> > This patch makes target-independent code use force_subreg instead
> > of simplify_gen_subreg in some places.  The criteria were:
> >
> > (1) The code is obviously specific to expand (where new pseudos
> >      can be created), or at least would be invalid to call when
> >      !can_create_pseudo_p () and temporaries are needed.
> >
> > (2) The value is obviously an rvalue rather than an lvalue.
> >
> > (3) The offset wasn't a simple lowpart or highpart calculation;
> >      a later patch will deal with those.
> >
> > Doing this should reduce the likelihood of bugs like PR115464
> > occuring in other situations.
> >
> > gcc/
> >       * expmed.cc (store_bit_field_using_insv): Use force_subreg
> >       instead of simplify_gen_subreg.
> >       (store_bit_field_1): Likewise.
> >       (extract_bit_field_as_subreg): Likewise.
> >       (extract_integral_bit_field): Likewise.
> >       (emit_store_flag_1): Likewise.
> >       * expr.cc (convert_move): Likewise.
> >       (convert_modes): Likewise.
> >       (emit_group_load_1): Likewise.
> >       (emit_group_store): Likewise.
> >       (expand_assignment): Likewise.
> [ ... ]
>
> So this has triggered a failure on ft32-elf with this testcase
> (simplified from the testsuite):
>
> typedef _Bool bool;
> const bool false = 0;
> const bool true = 1;
>
> struct RenderBox
> {
>    bool m_positioned : 1;
> };
>
> typedef struct RenderBox RenderBox;
>
>
> void RenderBox_setStyle(RenderBox *thisin)
> {
>    RenderBox *this = thisin;
>    bool ltrue = true;
>    this->m_positioned = ltrue;
>
> }
>
>
>
> Before this change we generated this:
>
> > (insn 13 12 14 (set (reg:QI 47)
> >         (mem/c:QI (plus:SI (reg/f:SI 37 virtual-stack-vars)
> >                 (const_int -5 [0xfffffffffffffffb])) [1 ltrue+0 S1 A8])) "j.c":17:22 -1
> >      (nil))
> >
> > (insn 14 13 15 (parallel [
> >             (set (zero_extract:SI (subreg:SI (reg:QI 46) 0)
> >                     (const_int 1 [0x1])
> >                     (const_int 0 [0]))
> >                 (subreg:SI (reg:QI 47) 0))
> >             (clobber (scratch:SI))
> >         ]) "j.c":17:22 -1
> >      (nil))
>
>
> Afterwards we generate:
>
> > (insn 13 12 14 2 (parallel [
> >             (set (zero_extract:SI (subreg:SI (reg:QI 46) 0)
> >                     (const_int 1 [0x1])
> >                     (const_int 0 [0]))
> >                 (subreg:SI (mem/c:QI (plus:SI (reg/f:SI 37 virtual-stack-vars)
> >                             (const_int -5 [0xfffffffffffffffb])) [1 ltrue+0 S1 A8]) 0))
> >             (clobber (scratch:SI))
> >         ]) "j.c":17:22 -1
> >      (nil))
>
> Note the (subreg (mem (...)).  Probably not desirable in general, but
> also note the virtual-stack-vars in the memory address.  The code to
> instantiate virtual registers doesn't handle (subreg (mem)), so we never
> convert that to an FP based address and we eventually fault.

We should really get rid of the support of `(subreg (mem))` as a valid
for register_operand (recorg.cc).
Two ideas on how to fix this before removing `(subreg (mem))` support
from register_operand:
1) Maybe for now reject subreg of mem inside validate_subreg that have
virtual-stack-vars addresses.
2) Or we add the code to instantiate virtual registers to handle (subreg (mem)).

Maybe we should just bite the bullet and remove support of `(subreg
(mem))` from register_operand instead of hacking around this
preexisting mess.

Also see
https://inbox.sourceware.org/gcc-patches/485B2857.2070003@naturalbridge.com/

Which is from 2008 about this subreg of mem.

Thanks,
Andrew

>
> Should be visible with ft32-elf cross compiler.  No options needed.
>
> Jeff
>
>

Jeff Law <jeffreyalaw@gmail.com> writes:
> On 6/17/24 3:53 AM, Richard Sandiford wrote:
>> This patch makes target-independent code use force_subreg instead
>> of simplify_gen_subreg in some places.  The criteria were:
>> 
>> (1) The code is obviously specific to expand (where new pseudos
>>      can be created), or at least would be invalid to call when
>>      !can_create_pseudo_p () and temporaries are needed.
>> 
>> (2) The value is obviously an rvalue rather than an lvalue.
>> 
>> (3) The offset wasn't a simple lowpart or highpart calculation;
>>      a later patch will deal with those.
>> 
>> Doing this should reduce the likelihood of bugs like PR115464
>> occuring in other situations.
>> 
>> gcc/
>> 	* expmed.cc (store_bit_field_using_insv): Use force_subreg
>> 	instead of simplify_gen_subreg.
>> 	(store_bit_field_1): Likewise.
>> 	(extract_bit_field_as_subreg): Likewise.
>> 	(extract_integral_bit_field): Likewise.
>> 	(emit_store_flag_1): Likewise.
>> 	* expr.cc (convert_move): Likewise.
>> 	(convert_modes): Likewise.
>> 	(emit_group_load_1): Likewise.
>> 	(emit_group_store): Likewise.
>> 	(expand_assignment): Likewise.
> [ ... ]
>
> So this has triggered a failure on ft32-elf with this testcase 
> (simplified from the testsuite):
>
> typedef _Bool bool;
> const bool false = 0;
> const bool true = 1;
>
> struct RenderBox
> {
>    bool m_positioned : 1;
> };
>
> typedef struct RenderBox RenderBox;
>
>
> void RenderBox_setStyle(RenderBox *thisin)
> {
>    RenderBox *this = thisin;
>    bool ltrue = true;
>    this->m_positioned = ltrue;
>
> }
>
>
>
> Before this change we generated this:
>
>> (insn 13 12 14 (set (reg:QI 47)
>>         (mem/c:QI (plus:SI (reg/f:SI 37 virtual-stack-vars)
>>                 (const_int -5 [0xfffffffffffffffb])) [1 ltrue+0 S1 A8])) "j.c":17:22 -1
>>      (nil))
>> 
>> (insn 14 13 15 (parallel [
>>             (set (zero_extract:SI (subreg:SI (reg:QI 46) 0)
>>                     (const_int 1 [0x1])
>>                     (const_int 0 [0]))
>>                 (subreg:SI (reg:QI 47) 0))
>>             (clobber (scratch:SI))
>>         ]) "j.c":17:22 -1
>>      (nil))
>
>
> Afterwards we generate:
>
>> (insn 13 12 14 2 (parallel [
>>             (set (zero_extract:SI (subreg:SI (reg:QI 46) 0)
>>                     (const_int 1 [0x1])
>>                     (const_int 0 [0]))
>>                 (subreg:SI (mem/c:QI (plus:SI (reg/f:SI 37 virtual-stack-vars)
>>                             (const_int -5 [0xfffffffffffffffb])) [1 ltrue+0 S1 A8]) 0))
>>             (clobber (scratch:SI))
>>         ]) "j.c":17:22 -1
>>      (nil))
>
> Note the (subreg (mem (...)).  Probably not desirable in general, but 
> also note the virtual-stack-vars in the memory address.  The code to 
> instantiate virtual registers doesn't handle (subreg (mem)), so we never 
> convert that to an FP based address and we eventually fault.
>
> Should be visible with ft32-elf cross compiler.  No options needed.

Bah.  Thanks for the report.

I agree of course with the follow-on discussion that we should get
rid of (subreg (mem)).  But this was supposed to be a conservative
patch.  I've therefore reverted the offending part of the commit,
as below.  (Tested on aarch64-linux-gnu.)

Richard


One of the changes in g:d4047da6a070175aae7121c739d1cad6b08ff4b2
caused a regression in ft32-elf; see:

    https://gcc.gnu.org/pipermail/gcc-patches/2024-June/655418.html

for details.  This change was different from the others in that the
original call was to simplify_subreg rather than simplify_lowpart_subreg.
The old code would therefore go on to do the force_reg for more cases
than the new code would.

gcc/
	* expmed.cc (store_bit_field_using_insv): Revert earlier change
	to use force_subreg instead of simplify_gen_subreg.
---
 gcc/expmed.cc | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/gcc/expmed.cc b/gcc/expmed.cc
index 3b9475f5aa0..8bbbc94a98c 100644
--- a/gcc/expmed.cc
+++ b/gcc/expmed.cc
@@ -695,7 +695,13 @@ store_bit_field_using_insv (const extraction_insn *insv, rtx op0,
 	     if we must narrow it, be sure we do it correctly.  */
 
 	  if (GET_MODE_SIZE (value_mode) < GET_MODE_SIZE (op_mode))
-	    tmp = force_subreg (op_mode, value1, value_mode, 0);
+	    {
+	      tmp = simplify_subreg (op_mode, value1, value_mode, 0);
+	      if (! tmp)
+		tmp = simplify_gen_subreg (op_mode,
+					   force_reg (value_mode, value1),
+					   value_mode, 0);
+	    }
 	  else
 	    {
 	      if (targetm.mode_rep_extended (op_mode, value_mode) != UNKNOWN)