Sent a new version for this in
https://gcc.gnu.org/pipermail/gcc-patches/2024-October/665794.html.
Thanks,
Konstantinos.
On Fri, Aug 30, 2024 at 4:32 PM Richard Sandiford
<richard.sandif...@arm.com> wrote:
>
> Manolis Tsamis <manolis.tsa...@vrull.eu> writes:
> >> > I could have some help with that, because after the new changes a
> >> > subreg related ICE also happens within store_bit_field when a DI ->
> >> > V4SI case is hit. Afaik store_bit_field should just return NULL if it
> >> > can't handle something so I don't really know how to address this ICE
> >> > in the new version.
> >>
> >> I don't think store_bit_field is expected to fail. But yeah, if you have
> >> a testcase, I can take a look.
> >>
> >
> > Yes, that was my initial reaction too. I don't yet have a reduced
> > testcase, this now only happens with SPEC2017 gcc_r when compiled with
> > `-g -O3 -march=armv8.2-a -flto=32 -favoid-store-forwarding` (doesn't
> > reproduce without flto). I have also attached the work-in-progress
> > patch that implements your changes up to that point and which can be
> > used to reproduce this, at least till we have a smaller testcase.
> >
> > The case that leads to that ICE is this:
> >
> > Store forwarding detected:
> > From: (insn 1700 1714 1701 132 (set (mem/j/c:SI (plus:DI (reg/f:DI 64 sfp)
> > (const_int -160 [0xffffffffffffff60])) [0 +0 S4 A128])
> > (reg:SI 617)) "real.c":158:9 69 {*movsi_aarch64}
> > (expr_list:REG_DEAD (reg:SI 617)
> > (nil)))
> > From: (insn 1695 1698 1715 132 (set (mem/c:TI (plus:DI (reg/f:DI 64 sfp)
> > (const_int -160 [0xffffffffffffff60])) [0 MEM [(void
> > *)&r]+0 S16 A128])
> > (const_int 0 [0])) "real.c":157:3 75 {*movti_aarch64}
> > (nil))
> > To: (insn 1709 1703 1696 132 (set (reg:V4SI 622 [ r ])
> > (mem/c:V4SI (plus:DI (reg/f:DI 64 sfp)
> > (const_int -160 [0xffffffffffffff60])) [51 r+0 S16
> > A128])) "builtins.c":9619:13 1270 {*aarch64_simd_movv4si}
> > (nil))
> >
> > Which is somewhat suspicious; we'll have to first make sure it's not a
> > avoid-store-forwarding bug.
>
> Does 1695 come before 1700? If so, it looks like it should be valid.
>
> > In any case the ICE is
> >
> > builtins.c:6684:1: internal compiler error: in simplify_subreg, at
> > simplify-rtx.cc:7680
> > 6684 | }
> > | ^
> > 0xc41ecb simplify_context::simplify_subreg(machine_mode, rtx_def*,
> > machine_mode, poly_int<2u, unsigned long>)
> > /home/mtsamis/src/gcc/gcc4/gcc/simplify-rtx.cc:7680
> > 0xc42d0f simplify_context::simplify_gen_subreg(machine_mode, rtx_def*,
> > machine_mode, poly_int<2u, unsigned long>)
> > /home/mtsamis/src/gcc/gcc4/gcc/simplify-rtx.cc:8007
> > 0x8afb4b simplify_gen_subreg(machine_mode, rtx_def*, machine_mode,
> > poly_int<2u, unsigned long>)
> > /home/mtsamis/src/gcc/gcc4/gcc/rtl.h:3562
> > 0x8afb4b force_subreg(machine_mode, rtx_def*, machine_mode,
> > poly_int<2u, unsigned long>)
> > /home/mtsamis/src/gcc/gcc4/gcc/explow.cc:755
> > 0x8b9ef3 store_bit_field_1
> > /home/mtsamis/src/gcc/gcc4/gcc/expmed.cc:810
> > 0x8ba327 store_bit_field(rtx_def*, poly_int<2u, unsigned long>,
> > poly_int<2u, unsigned long>, poly_int<2u, unsigned long>, poly_int<2u,
> > unsigned long>, machine_mode, rtx_def*, bool, bool)
> > /home/mtsamis/src/gcc/gcc4/gcc/expmed.cc:1214
> > 0xc53c7f generate_bit_insert_sequence
> > /home/mtsamis/src/gcc/gcc4/gcc/avoid-store-forwarding.cc:122
> > 0xc53c7f process_store_forwarding
> > /home/mtsamis/src/gcc/gcc4/gcc/avoid-store-forwarding.cc:231
> > 0xc53c7f avoid_store_forwarding
> > /home/mtsamis/src/gcc/gcc4/gcc/avoid-store-forwarding.cc:504
> > 0xc546eb execute
> > /home/mtsamis/src/gcc/gcc4/gcc/avoid-store-forwarding.cc:571
>
> OK, thanks, I'll have a go at reproducing, but it sounds like it might
> be tricky. :)
>
> >> > So, although I don't like it and it complicates things, I had to add
> >> > recog_memoized pretty much everywhere and also make sure to only emit
> >> > if all of these are successful...
> >>
> >> Do you have testcases? emit_move_insn's job is to make the move
> >> legitimate, so it seems like the checks might be papering over an issue
> >> elsewhere.
> >>
> > I had a quick look and couldn't find an existing testcase. I'll have
> > to recheck more thoroughly in order to create a testcase (if possible
> > at all).
> > I remember that in one case the rtx provided was zero_extend and there
> > was no instruction for that so recog would fail.
>
> Ah, ok. That might have been the bug then. emit_move_insn should only
> be used for moving one object to another, where an "object" can be a
> constant, memory or register (including subregs of registers).
> The advantage of using it is that it works out how to make the move
> valid, given target limitations.
>
> If the instruction can be more general than that then we should just use
> gen_rtx_SET, and recog the result.
>
> Thanks,
> Richard
