Re: [PATCH 3/8] Make more use of force_subreg

[Andrew Pinski <pinskia@gmail.com>]

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
>
>