On Wed, 2023-08-02 at 12:54 -0400, Vladimir Makarov wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
>
> On 7/17/23 07:33, [email protected] wrote:
> > Hi,
> >
> > The avr target has a bunch of patterns that directly set hard regs at
> > expand time, like so
> >
> > (define_expand "cpymemhi"
> > [(parallel [(set (match_operand:BLK 0 "memory_operand" "")
> > (match_operand:BLK 1 "memory_operand" ""))
> > (use (match_operand:HI 2 "const_int_operand" ""))
> > (use (match_operand:HI 3 "const_int_operand" ""))])]
> > ""
> > {
> > if (avr_emit_cpymemhi (operands))
> > DONE;
> >
> > FAIL;
> > })
> >
> > where avr_emit_cpymemhi generates
> >
> > (insn 14 13 15 4 (set (reg:HI 30 r30)
> > (reg:HI 48 [ ivtmp.10 ])) "pr53505.c":21:22 -1
> > (nil))
> > (insn 15 14 16 4 (set (reg:HI 26 r26)
> > (reg/f:HI 38 virtual-stack-vars)) "pr53505.c":21:22 -1
> > (nil))
> > (insn 16 15 17 4 (parallel [
> > (set (mem:BLK (reg:HI 26 r26) [0 A8])
> > (mem:BLK (reg:HI 30 r30) [0 A8]))
> > (unspec [
> > (const_int 0 [0])
> > ] UNSPEC_CPYMEM)
> > (use (reg:QI 52))
> > (clobber (reg:HI 26 r26))
> > (clobber (reg:HI 30 r30))
> > (clobber (reg:QI 0 r0))
> > (clobber (reg:QI 52))
> > ]) "pr53505.c":21:22 -1
> > (nil))
> >
> > Classic reload knows about these - find_reg masks out bad_spill_regs, and
> > bad_spill_regs
> > when ORed with chain->live_throughout in order_regs_for_reload picks up r30.
> >
> > LRA, however, appears to not consider that, and proceeds to use such regs
> > as reload regs.
> > For the same source, it generates
> > <snip>
> > Choosing alt 0 in insn 15: (0) =r (1) r {*movhi_split}
> > Creating newreg=70, assigning class GENERAL_REGS to r70
> > 15: r26:HI=r70:HI
> > REG_EQUAL r28:HI+0x1
> > Inserting insn reload before:
> > 58: r70:HI=r28:HI+0x1
> >
> > Choosing alt 3 in insn 58: (0) d (1) 0 (2) nYnn {*addhi3_split}
> > Creating newreg=71 from oldreg=70, assigning class LD_REGS to r71
> > 58: r71:HI=r71:HI+0x1
> > Inserting insn reload before:
> > 59: r71:HI=r28:HI
> > Inserting insn reload after:
> > 60: r70:HI=r71:HI
> >
> > ********** Assignment #1: **********
> >
> > Assigning to 71 (cl=LD_REGS, orig=70, freq=3000, tfirst=71,
> > tfreq=3000)...
> > Assign 30 to reload r71 (freq=3000)
> > Hard reg 26 is preferable by r70 with profit 1000
> > Hard reg 30 is preferable by r70 with profit 1000
> > Assigning to 70 (cl=GENERAL_REGS, orig=70, freq=2000, tfirst=70,
> > tfreq=2000)...
> > Assign 30 to reload r70 (freq=2000)
> >
> >
> > (insn 14 13 59 3 (set (reg:HI 30 r30)
> > (reg:HI 18 r18 [orig:48 ivtmp.10 ] [48])) "pr53505.c":21:22 101
> > {*movhi_split}
> > (nil))
> > (insn 59 14 58 3 (set (reg:HI 30 r30 [70])
> > (reg/f:HI 28 r28)) "pr53505.c":21:22 101 {*movhi_split}
> > (nil))
> > (insn 58 59 15 3 (set (reg:HI 30 r30 [70])
> > (plus:HI (reg:HI 30 r30 [70])
> > (const_int 1 [0x1]))) "pr53505.c":21:22 165 {*addhi3_split}
> > (nil))
> > (insn 15 58 16 3 (set (reg:HI 26 r26)
> > (reg:HI 30 r30 [70])) "pr53505.c":21:22 101 {*movhi_split}
> > (expr_list:REG_EQUAL (plus:HI (reg/f:HI 28 r28)
> > (const_int 1 [0x1]))
> > (nil)))
> > (insn 16 15 17 3 (parallel [
> > (set (mem:BLK (reg:HI 26 r26) [0 A8])
> > (mem:BLK (reg:HI 30 r30) [0 A8]))
> > (unspec [
> > (const_int 0 [0])
> > ] UNSPEC_CPYMEM)
> > (use (reg:QI 22 r22 [52]))
> > (clobber (reg:HI 26 r26))
> > (clobber (reg:HI 30 r30))
> > (clobber (reg:QI 0 r0))
> > (clobber (reg:QI 22 r22 [52]))
> > ]) "pr53505.c":21:22 132 {cpymem_qi}
> > (nil))
> >
> > LRA generates insn 59 that clobbers r30 set in insn 14, causing an execution
> > failure down the line.
> >
> > How should the avr backend deal with this?
> >
> Sorry for the big delay with the answer. I was on vacation.
>
> There are probably some ways to fix it by changing patterns as other
> people suggested but I'd like to see the current patterns work for LRA
> as well.
>
> Could you send me the test case on which I could reproduce the problem
> and work on implementing such functionality.
>
>
Thanks for taking your time to look at this.
To reproduce the behavior, apply the below patch on master
diff --git gcc/config/avr/avr.cc gcc/config/avr/avr.cc
index 25f3f4c22e0..a9ab8259339 100644
--- gcc/config/avr/avr.cc
+++ gcc/config/avr/avr.cc
@@ -1574,6 +1574,9 @@ avr_allocate_stack_slots_for_args (void)
static bool
avr_can_eliminate (const int from ATTRIBUTE_UNUSED, const int to)
{
+ if (lra_in_progress && to == STACK_POINTER_REGNUM)
+ return false;
+
return ((frame_pointer_needed && to == FRAME_POINTER_REGNUM)
|| !frame_pointer_needed);
}
@@ -15246,9 +15249,6 @@ avr_float_lib_compare_returns_bool (machine_mode mode,
enum rtx_code)
#undef TARGET_CONVERT_TO_TYPE
#define TARGET_CONVERT_TO_TYPE avr_convert_to_type
-#undef TARGET_LRA_P
-#define TARGET_LRA_P hook_bool_void_false
-
#undef TARGET_ADDR_SPACE_SUBSET_P
#define TARGET_ADDR_SPACE_SUBSET_P avr_addr_space_subset_p
diff --git gcc/config/avr/avr.h gcc/config/avr/avr.h
index 8e7e00db13b..f82f429b94a 100644
--- gcc/config/avr/avr.h
+++ gcc/config/avr/avr.h
@@ -313,8 +313,7 @@ enum reg_class {
#define ELIMINABLE_REGS { \
{ ARG_POINTER_REGNUM, STACK_POINTER_REGNUM }, \
{ ARG_POINTER_REGNUM, FRAME_POINTER_REGNUM }, \
- { FRAME_POINTER_REGNUM, STACK_POINTER_REGNUM }, \
- { FRAME_POINTER_REGNUM + 1, STACK_POINTER_REGNUM + 1 } }
+ { FRAME_POINTER_REGNUM, STACK_POINTER_REGNUM } }
#define INITIAL_ELIMINATION_OFFSET(FROM, TO, OFFSET) \
OFFSET = avr_initial_elimination_offset (FROM, TO)
Then configure and build for avr without attempting to build libgcc
$ configure --target=avr --prefix=<prefix_dir> --enable-languages=c && make
all-host && make install-host
And finally to reproduce the failure
$ cat test.c
#include <stdbool.h>
struct A
{
unsigned int a;
unsigned char c1, c2;
bool b1 : 1;
};
void
foo (const struct A *x, int y)
{
int s = 0, i;
for (i = 0; i < y; ++i)
{
const struct A a = x[i];
s += a.b1 ? 1 : 0;
}
if (s != 0)
__builtin_abort ();
}
$ avr-gcc -mmcu=avr51 -Os test.c -S -o - -fdump-rtl-all
You can find the below insn sequence in test.c.*.reload
(insn 13 12 14 3 (set (reg:QI 22 r22 [52])
(const_int 5 [0x5])) "test.c":16:22 86 {movqi_insn_split}
(expr_list:REG_EQUAL (const_int 5 [0x5])
(nil)))
(insn 14 13 59 3 (set (reg:HI 30 r30)
(reg:HI 18 r18 [orig:48 ivtmp.9 ] [48])) "test.c":16:22 101
{*movhi_split}
(nil))
(insn 59 14 58 3 (set (reg:HI 30 r30 [70])
(reg/f:HI 28 r28)) "test.c":16:22 101 {*movhi_split}
(nil))
(insn 58 59 15 3 (set (reg:HI 30 r30 [70])
(plus:HI (reg:HI 30 r30 [70])
(const_int 1 [0x1]))) "test.c":16:22 165 {*addhi3_split}
(nil))
(insn 15 58 16 3 (set (reg:HI 26 r26)
(reg:HI 30 r30 [70])) "test.c":16:22 101 {*movhi_split}
(expr_list:REG_EQUAL (plus:HI (reg/f:HI 28 r28)
(const_int 1 [0x1]))
(nil)))
(insn 16 15 17 3 (parallel [
(set (mem:BLK (reg:HI 26 r26) [0 A8])
(mem:BLK (reg:HI 30 r30) [0 A8]))
(unspec [
(const_int 0 [0])
] UNSPEC_CPYMEM)
(use (reg:QI 22 r22 [52]))
(clobber (reg:HI 26 r26))
(clobber (reg:HI 30 r30))
(clobber (reg:QI 0 r0))
(clobber (reg:QI 22 r22 [52]))
]) "test.c":16:22 132 {cpymem_qi}
(nil))
Classic reload produces
(insn 13 12 14 3 (set (reg:QI 22 r22 [52])
(const_int 5 [0x5])) "test.c":16:22 86 {movqi_insn_split}
(expr_list:REG_EQUAL (const_int 5 [0x5])
(nil)))
(insn 14 13 59 3 (set (reg:HI 30 r30)
(reg:HI 18 r18 [orig:48 ivtmp.9 ] [48])) "test.c":16:22 101
{*movhi_split}
(nil))
(insn 59 14 15 3 (set (reg:HI 26 r26)
(reg/f:HI 28 r28)) "test.c":16:22 101 {*movhi_split}
(nil))
(insn 15 59 16 3 (set (reg:HI 26 r26)
(plus:HI (reg:HI 26 r26)
(const_int 1 [0x1]))) "test.c":16:22 165 {*addhi3_split}
(expr_list:REG_EQUAL (plus:HI (reg/f:HI 28 r28)
(const_int 1 [0x1]))
(nil)))
(insn 16 15 17 3 (parallel [
(set (mem:BLK (reg:HI 26 r26) [0 A8])
(mem:BLK (reg:HI 30 r30) [0 A8]))
(unspec [
(const_int 0 [0])
] UNSPEC_CPYMEM)
(use (reg:QI 22 r22 [52]))
(clobber (reg:HI 26 r26))
(clobber (reg:HI 30 r30))
(clobber (reg:QI 0 r0))
(clobber (reg:QI 22 r22 [52]))
]) "test.c":16:22 132 {cpymem_qi}
(nil))
Regards
Senthil
