On Wed, Nov 20, 2024 at 11:29 AM Georg-Johann Lay via Gcc
<[email protected]> wrote:
>
> Consider the following RTL peephole from avr.md:
>
> (define_peephole2 ; avr.md:5387
> [(match_scratch:QI 3 "d")
> (parallel [(set (match_operand:ALL4 0 "register_operand" "")
> (ashift:ALL4 (match_operand:ALL4 1
> "register_operand" "")
> (match_operand:QI 2 "const_int_operand"
> "")))
> (clobber (reg:CC REG_CC))])]
> ""
> [(parallel [(set (match_dup 0)
> (ashift:ALL4 (match_dup 1)
> (match_dup 2)))
> (clobber (match_dup 3))
> (clobber (reg:CC REG_CC))])])
>
> As far as I understand, its purpose is to provide a QImode
> scratch register provided such a scratch is available.
>
> However, in the .peephole2 RTL dump with -da I see the following:
>
> Splitting with gen_peephole2_100 (avr.md:5387)
> ...
> (insn 24 8 15 2 (parallel [
> (set (reg:SI 22 r22 [orig:47 _3 ] [47])
> (ashift:SI (reg:SI 20 r20 [orig:48 x ] [48])
> (const_int 7 [0x7])))
> (clobber (reg:QI 24 r24))
> (clobber (reg:CC 36 cc))
> ])
> (nil))
>
> That is, the scratch r24:QI is overlapping the output in
> r22:SI. All hard registers are 8-bit regs and hence r22:SI
> extends from r22...r25.
>
> A scratch that overlaps the operands is pretty much useless
> or even plain wrong. recog.cc::peep2_find_free_register()
> has this comment: /* Don't use registers set or clobbered by the insn. */
>
> from = peep2_buf_position (peep2_current + from);
> to = peep2_buf_position (peep2_current + to);
>
> gcc_assert (peep2_insn_data[from].insn != NULL_RTX);
> REG_SET_TO_HARD_REG_SET (live, peep2_insn_data[from].live_before);
>
> while (from != to)
> {
> gcc_assert (peep2_insn_data[from].insn != NULL_RTX);
>
> /* Don't use registers set or clobbered by the insn. */
> FOR_EACH_INSN_DEF (def, peep2_insn_data[from].insn)
> SET_HARD_REG_BIT (live, DF_REF_REGNO (def));
>
> from = peep2_buf_position (from + 1);
> }
>
> So it this bogus in that it assumes all registers extend only
> over one hard reg?
Yes, looks like a bug to me.
>
> FYI, the purpose is to provide a scratch without increasing the register
> pressure (which "match_scratch" would do). Therefore, the RTL peephole
> is used instead of forcing reload to come up with a scratch.
>
> More specifically, I see this with
>
> $ avr-gcc bogus-peep2.c -S -Os -da
>
> long ashl32_7 (int i, long x)
> {
> return x << 7;
> }
>
> with the attached WIP patch atop trunk b222ee10045d.
>
> Johann
>
> Target: avr
> Configured with: ../../source/gcc-master/configure --target=avr
> --disable-nls --with-dwarf2 --with-gnu-as --with-gnu-ld --disable-shared
> --enable-languages=c,c++
> Thread model: single
> Supported LTO compression algorithms: zlib
> gcc version 15.0.0 20241119 (experimental) (GCC)
