On Fri, Jun 21, 2024 at 1:11 PM Jeff Law <jeffreya...@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.2070...@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
>
>
