On Fri, 2023-07-14 at 09:29 -0400, Vladimir Makarov wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the > content is safe > > On 7/13/23 05:27, SenthilKumar.Selvaraj--- via Gcc wrote: > > Hi, > > > > I've been spending some (spare) time checking what it would take to > > make LRA work for the avr target. > > > > Right after I removed the TARGET_LRA_P hook disabling LRA, building > > libgcc failed with a weird ICE. > > On the avr, the stack pointer (SP) > > is not used to access stack slots > It is very uncommon target then. > > - TARGET_CAN_ELIMINATE returns false > > if frame_pointer_needed, and TARGET_FRAME_POINTER_REQUIRED returns true > > if get_frame_size() > 0. > > > > With LRA, however, reload generates > > > > (insn 159 239 240 7 (set (mem/c:QI (plus:HI (reg/f:HI 32 __SP_L__) > > (const_int 1 [0x1])) [2 %sfp+1 S1 A8]) > > (reg:QI 24 r24 [orig:86 a ] [86])) "case.c":7:7 86 > > {movqi_insn_split} > > (nil)) > > > > and the backend code errors out when it finds SP is being used as a > > pointer register. > > > > Digging through the RTL dumps, I found the following. For the > > following insn sequence in *.ira > > > > (insn 189 128 159 7 (set (reg:HI 58 [ b ]) > > (const_int 0 [0])) "case.c":7:7 101 {*movhi_split} > > (nil)) > > (insn 159 189 160 7 (set (subreg:QI (reg:HI 58 [ b ]) 0) > > (reg:QI 86 [ a ])) "case.c":7:7 86 {movqi_insn_split} > > (nil)) > > (insn 160 159 32 7 (set (subreg:QI (reg:HI 58 [ b ]) 1) > > (reg:QI 87 [ a+1 ])) "case.c":7:7 86 {movqi_insn_split} > > (nil)) > > > > 1. For r58, IRA picks R28:R29, which is the frame pointer for avr. > > > > Popping a13(r58,l0) -- assign reg 28 > > > > 2. LRA sees the subreg in insn 159 and generates a reload reg > > (r125). simplify_subreg_regno (lra-constraints.cc:1810) however > > bails (returns -1) if the reg involved is FRAME_POINTER_REGNUM and > > reload isn't completed yet. LRA therefore decides rclass for the > > pseudo reg is NO_REGS. > > > > <snip> > > Creating newreg=125 from oldreg=58, assigning class NO_REGS to subreg reg > > r125 > > 159: r125:HI#0=r86:QI > > > > 4. As rclass is NO_REGS, LRA picks an insn alternative that involves > > memory. > > That is my understanding, please correct me if I'm wrong. > > <snip> > > 0 Small class reload: reject+=3 > > 0 Non input pseudo reload: reject++ > > Cycle danger: overall += LRA_MAX_REJECT > > alt=0,overall=610,losers=1,rld_nregs=1 > > 0 Small class reload: reject+=3 > > 0 Non input pseudo reload: reject++ > > alt=1: Bad operand -- refuse > > 0 Non pseudo reload: reject++ > > alt=2,overall=1,losers=0,rld_nregs=0 > > Choosing alt 2 in insn 159: (0) Qm (1) rY00 {movqi_insn_split} > > > > 5. LRA creates stack slots, and then uses the FP register to access > > the slots. This is despite r58 already being assigned R28:R29. > > > > 6. TARGET_FRAME_POINTER_REQUIRED is never called, and therefore > > frame_pointer_needed is not set, despite the creation of stack > > slots. TARGET_CAN_ELIMINATE therefore okays elimination of FP to SP, > > and this eventually causes the ICE when the avr backend sees SP being > > used as a pointer register. > > > > This is the relevant sequence after reload > > <snip> > > (insn 189 128 239 7 (set (reg:HI 28 r28 [orig:58 b ] [58]) > > (const_int 0 [0])) "case.c":7:7 101 {*movhi_split} > > (nil)) > > (insn 239 189 159 7 (set (mem/c:HI (plus:HI (reg/f:HI 32 __SP_L__) > > (const_int 1 [0x1])) [2 %sfp+1 S2 A8]) > > (reg:HI 28 r28 [orig:58 b ] [58])) "case.c":7:7 101 {*movhi_split} > > (nil)) > > (insn 159 239 240 7 (set (mem/c:QI (plus:HI (reg/f:HI 32 __SP_L__) > > (const_int 1 [0x1])) [2 %sfp+1 S1 A8]) > > (reg:QI 24 r24 [orig:86 a ] [86])) "case.c":7:7 86 > > {movqi_insn_split} > > (nil)) > > (insn 240 159 241 7 (set (reg:HI 28 r28 [orig:58 b ] [58]) > > (mem/c:HI (plus:HI (reg/f:HI 32 __SP_L__) > > (const_int 1 [0x1])) [2 %sfp+1 S2 A8])) "case.c":7:7 101 > > {*movhi_split} > > (nil)) > > (insn 241 240 160 7 (set (mem/c:HI (plus:HI (reg/f:HI 32 __SP_L__) > > (const_int 1 [0x1])) [2 %sfp+1 S2 A8]) > > (reg:HI 28 r28 [orig:58 b ] [58])) "case.c":7:7 101 {*movhi_split} > > (nil)) > > (insn 160 241 242 7 (set (mem/c:QI (plus:HI (reg/f:HI 32 __SP_L__) > > (const_int 2 [0x2])) [2 %sfp+2 S1 A8]) > > (reg:QI 18 r18 [orig:87 a+1 ] [87])) "case.c":7:7 86 > > {movqi_insn_split} > > (nil)) > > (insn 242 160 33 7 (set (reg:HI 28 r28 [orig:58 b ] [58]) > > (mem/c:HI (plus:HI (reg/f:HI 32 __SP_L__) > > (const_int 1 [0x1])) [2 %sfp+1 S2 A8])) "case.c":7:7 101 > > {*movhi_split} > > (nil)) > > > > For choices other than FP, simplify_subreg_regno returns the correct part > > of the wider HImode reg, so rclass is not NO_REGS, and things workout > > fine. > > > > I checked what classic reload does in the same situation - it picks a > > different register (R25) instead of spilling to a stack slot. > > > > <snip> > > (insn 189 128 159 7 (set (reg:HI 28 r28 [orig:58 b ] [58]) > > (const_int 0 [0])) "case.c":7:7 101 {*movhi_split} > > (nil)) > > (insn 159 189 226 7 (set (reg:QI 25 r25) > > (reg:QI 24 r24 [orig:86 a ] [86])) "case.c":7:7 86 > > {movqi_insn_split} > > (nil)) > > (insn 226 159 160 7 (set (reg:QI 28 r28) > > (reg:QI 25 r25)) "case.c":7:7 86 {movqi_insn_split} > > (nil)) > > (insn 160 226 227 7 (set (reg:QI 25 r25) > > (reg:QI 18 r18 [orig:87 a+1 ] [87])) "case.c":7:7 86 > > {movqi_insn_split} > > (nil)) > > (insn 227 160 33 7 (set (reg:QI 29 r29) > > (reg:QI 25 r25)) "case.c":7:7 86 {movqi_insn_split} > > (nil)) > > > > > > My questions: > > > > 1. Is there something obvious the avr backend is doing wrong that is > > causing this? > No. In my opinion if it worked with reload, it should work with LRA > because LRA is a substitute for reload. > > 2. Shouldn't LRA ask the backend for frame_pointer_required_p and > > update frame_pointer_needed if it creates stack slots? > I think so. > > 3. Even if (2) works, I see that lra-eliminates.cc:update_reg_eliminate > > asserts that if the backend said elimination to SP is > > ok first up, it > > cannot reject that elimination later (line 1165). If the only reason > > FP is required is because LRA created stack > > slots, what should the backend do? > > > I think it can be relaxed for avr on which sp can not be used access > stack memory > > 4. When simplify_subreg_regno bails for FP, lra-constraints.cc:1815 > > sets rclass = NO_REGS and forces a spill to memory. The comment says > > it is to prevent infinite looping, but for this case, doesn't it > > make sense to look for other regs? > > > I guess it can be relaxed for avr and frame-pointer too as avr does not > use sp for accessing memory > > 5. I can work around the problem by disabling elimination from FP to SP > > when lra_in_progress, but I think it pevents IRA/LRA from using > > R28:R29 even when FP is not required at all? > Yes, it is better to avoid disabling elimination > > 6. Basic question, but does FP to SP elimination mean any operation > > possible with FP should be doable with SP as well? > > Hard to say. There are a lot of undocumented RA assumptions. > > If you send me the preprocessed test, I could start to work on it to fix > the problems. I think it is hard to fix them right for a person having > a little experience with LRA. > > Ok, this is a reduced test case that reproduces the failure.
$ cat case.c typedef int HItype __attribute__ ((mode (HI))); HItype __mulvhi3 (HItype a, HItype b) { HItype w; if (__builtin_mul_overflow (a, b, &w)) __builtin_trap (); return w; } On latest master, this trivial patch turns on LRA for avr --- gcc/config/avr/avr.cc +++ gcc/config/avr/avr.cc @@ -15244,9 +15244,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 Then configuring and building 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 $ <prefix_dir>/bin/avr-gcc -mmcu=avr25 case.c -Os Thanks for taking this up - please do let me know if you need more information. Regards Senthil