On Mon, Aug 04, 2025 at 01:04:36PM +0200, Georg-Johann Lay wrote:
> Rewriting avr.md so it uses less explicit hard registers, I came
> across the ICE below. I am using 2 patches as attached:
>
> - A tentative fix for LRA https://gcc.gnu.org/PR121198
>
> - A WIP to rewrite avr.md's divmodhi4 to use HRCs instead of
> explicit hard regs. Outcome is that divmodhi4 will read
> like this:
>
> (define_insn_and_split "divmodhi4"
> [(set (match_operand:HI 0 "register_operand" "={r22}")
> (div:HI (match_operand:HI 1 "register_operand" "{r24}")
> (match_operand:HI 2 "register_operand" "{r22}")))
> (set (match_operand:HI 3 "register_operand" "={r24}")
> (mod:HI (match_dup 1)
> (match_dup 2)))
> (clobber (match_scratch:HI 4 "={r26}"))
> (clobber (match_scratch:QI 5 "={r21}"))]
> ""
> "#"
> "&& reload_completed"
> [(scratch)]
> { DONE_ADD_CCC })
>
> Then compile
>
> void bar (int, char*);
>
> void foo (int k)
> {
> bar (k / 5, "one");
> }
>
> $ avr-gcc spill.c -S -mmcu=avr4 -Os -da
>
> spill.c: In function 'foo':
> spill.c:6:1: error: unable to find a register to spill
> 6 | }
> | ^
> spill.c:6:1: error: this is the insn:
> (insn 9 20 16 2 (parallel [
> (set (reg:HI 53)
> (div:HI (reg:HI 49 [ k ])
> (reg:HI 54 [48])))
> (set (reg:HI 55 [47])
> (mod:HI (reg:HI 49 [ k ])
> (reg:HI 54 [48])))
> (clobber (reg:HI 56 [50]))
> (clobber (reg:QI 57 [51]))
> ]) "spill.c":5:5 588 {divmodhi4}
> (expr_list:REG_UNUSED (reg:QI 57 [51])
> (expr_list:REG_UNUSED (reg:HI 56 [50])
> (expr_list:REG_UNUSED (reg:HI 55 [47])
> (expr_list:REG_DEAD (reg:HI 54 [48])
> (expr_list:REG_DEAD (reg:HI 49 [ k ])
> (nil)))))))
> during RTL pass: reload
> dump file: spill.c.326r.reload
> spill.c:6:1: internal compiler error: in lra_split_hard_reg_for, at
> lra-assigns.cc:1863
>
> Some questions:
>
> 1) Is there anything wrong with the divmodhi4 pattern per se?
>
> AFAIU match_dup is the right thing to do. HImode regs must start
> in even registers, so that's fine too I think.
>
> When the pattern itself is fine, I would open a PR for the ICE.
>
> 2) I had a look at the IRA dump. The assignments it comes up with
> have nothing to do with the constraints:
>
> divmodhi4 Regs before IRA
> Op Reg IRA *=pseudo assigns
> 0 =r22:HI 24
> 1 r24:HI 49* 24
> 2 r22:HI 48* 18
> 3 =r24:HI 47* 18
> 4 =r26:HI 50* 20
> 5 =r21:QI 51* 30
>
> That is: IRA comes up with assigns that have nothing to do with
> the constraints of the insn. Doesn't that make it extra hard
> for LRA to come up with the specified constraints?
>
> IIUC, HRCs materialize in lra-constraints.cc, and IRA doesn't
> know anything about them. Isn't that a perfect situation for
> sub-optimal code? Since LRA will have to spill many of IRA's
> choices, requiring a zoo off extra moves and frame locations
> when reg pressure is high.
That is a nice finding. While implementing hard register constraints in
LRA I was looking for examples were it would have made sense to already
instruct IRA about preferred registers. Since everything could be fixed
up by LRA, I didn't want to prematurely implement something in IRA which
does not lead to better code and I never went on with that. My gut
feeling is that we see this especially on AVR since there values are
held in register pairs of more than two registers. So fixing it up
later is not that easy anymore if register pressure is high. I will
look more closely into this the coming days.
Cheers,
Stefan
>
> So I guess the problem is that IRA only works on the level of
> register classes, and it cannot do anything about HRCs since
> they don't have a register class in general?
>
> Johann
> diff --git a/gcc/lra-constraints.cc b/gcc/lra-constraints.cc
> index 83f8fda3b52..0c09c9aa45b 100644
> --- a/gcc/lra-constraints.cc
> +++ b/gcc/lra-constraints.cc
> @@ -2551,7 +2551,7 @@ process_alt_operands (int only_alternative)
> {
> int regno = decode_hard_reg_constraint (p);
> gcc_assert (regno >= 0);
> - cl = REGNO_REG_CLASS (regno);
> + cl = GENERAL_REGS;
> CLEAR_HARD_REG_SET (hard_reg_constraint);
> SET_HARD_REG_BIT (hard_reg_constraint, regno);
> cl_filter = &hard_reg_constraint;
> diff --git a/gcc/config/avr/avr.md b/gcc/config/avr/avr.md
> index 13493ad6891..496f70521ec 100644
> --- a/gcc/config/avr/avr.md
> +++ b/gcc/config/avr/avr.md
> @@ -3773,32 +3773,14 @@ (define_insn "*udivmodqi4_call"
> [(set_attr "type" "xcall")])
>
> (define_insn_and_split "divmodhi4"
> - [(set (match_operand:HI 0 "pseudo_register_operand")
> - (div:HI (match_operand:HI 1 "pseudo_register_operand")
> - (match_operand:HI 2 "pseudo_register_operand")))
> - (set (match_operand:HI 3 "pseudo_register_operand")
> - (mod:HI (match_dup 1) (match_dup 2)))
> - (clobber (reg:QI 21))
> - (clobber (reg:HI 22))
> - (clobber (reg:HI 24))
> - (clobber (reg:HI 26))]
> - ""
> - { gcc_unreachable(); }
> - ""
> - [(set (reg:HI 24) (match_dup 1))
> - (set (reg:HI 22) (match_dup 2))
> - (parallel [(set (reg:HI 22) (div:HI (reg:HI 24) (reg:HI 22)))
> - (set (reg:HI 24) (mod:HI (reg:HI 24) (reg:HI 22)))
> - (clobber (reg:HI 26))
> - (clobber (reg:QI 21))])
> - (set (match_dup 0) (reg:HI 22))
> - (set (match_dup 3) (reg:HI 24))])
> -
> -(define_insn_and_split "*divmodhi4_call_split"
> - [(set (reg:HI 22) (div:HI (reg:HI 24) (reg:HI 22)))
> - (set (reg:HI 24) (mod:HI (reg:HI 24) (reg:HI 22)))
> - (clobber (reg:HI 26))
> - (clobber (reg:QI 21))]
> + [(set (match_operand:HI 0 "register_operand" "={r22}")
> + (div:HI (match_operand:HI 1 "register_operand" "{r24}")
> + (match_operand:HI 2 "register_operand" "{r22}")))
> + (set (match_operand:HI 3 "register_operand" "={r24}")
> + (mod:HI (match_dup 1)
> + (match_dup 2)))
> + (clobber (match_scratch:HI 4 "={r26}"))
> + (clobber (match_scratch:QI 5 "={r21}"))]
> ""
> "#"
> "&& reload_completed"
> @@ -3816,32 +3798,14 @@ (define_insn "*divmodhi4_call"
> [(set_attr "type" "xcall")])
>
> (define_insn_and_split "udivmodhi4"
> - [(set (match_operand:HI 0 "pseudo_register_operand")
> - (udiv:HI (match_operand:HI 1 "pseudo_register_operand")
> - (match_operand:HI 2 "pseudo_register_operand")))
> - (set (match_operand:HI 3 "pseudo_register_operand")
> - (umod:HI (match_dup 1) (match_dup 2)))
> - (clobber (reg:QI 21))
> - (clobber (reg:HI 22))
> - (clobber (reg:HI 24))
> - (clobber (reg:HI 26))]
> - ""
> - { gcc_unreachable(); }
> - ""
> - [(set (reg:HI 24) (match_dup 1))
> - (set (reg:HI 22) (match_dup 2))
> - (parallel [(set (reg:HI 22) (udiv:HI (reg:HI 24) (reg:HI 22)))
> - (set (reg:HI 24) (umod:HI (reg:HI 24) (reg:HI 22)))
> - (clobber (reg:HI 26))
> - (clobber (reg:QI 21))])
> - (set (match_dup 0) (reg:HI 22))
> - (set (match_dup 3) (reg:HI 24))])
> -
> -(define_insn_and_split "*udivmodhi4_call_split"
> - [(set (reg:HI 22) (udiv:HI (reg:HI 24) (reg:HI 22)))
> - (set (reg:HI 24) (umod:HI (reg:HI 24) (reg:HI 22)))
> - (clobber (reg:HI 26))
> - (clobber (reg:QI 21))]
> + [(set (match_operand:HI 0 "register_operand" "={r22}")
> + (udiv:HI (match_operand:HI 1 "register_operand" "{r24}")
> + (match_operand:HI 2 "register_operand" "{r22}")))
> + (set (match_operand:HI 3 "register_operand" "={r24}")
> + (umod:HI (match_dup 1)
> + (match_dup 2)))
> + (clobber (match_scratch:HI 4 "={r26}"))
> + (clobber (match_scratch:QI 5 "={r21}"))]
> ""
> "#"
> "&& reload_completed"
> @@ -3853,8 +3817,7 @@ (define_insn "*udivmodhi4_call"
> (set (reg:HI 24) (umod:HI (reg:HI 24) (reg:HI 22)))
> (clobber (reg:HI 26))
> (clobber (reg:QI 21))
> - (clobber (reg:CC REG_CC))
> - ]
> + (clobber (reg:CC REG_CC))]
> "reload_completed"
> "%~call __udivmodhi4"
> [(set_attr "type" "xcall")])
