On Thu, Jan 15, 2026 at 09:40:21PM +0100, Stefan Schulze Frielinghaus wrote: > I was almost about to push v2 and then realized there is a flaw in my > design. Instead of checking whether the initial insns i3 to i0 are > making use of hard register constraints, I have to check whether the > resulting insn does. Or in other words, if an initial insn does make > use of hard register constraints and the resulting insn does not, then a > combination cannot introduce a conflict and is therefore ok. However, > conversely, if all initial insns do not make use of hard register > constraints, and therefore would pass the check of patch v2, the > resulting insn could make use of hard register constraints, possibly > resulting in a conflict. > > Long story short: do the check for hard register constraints for the > resulting insn only. > > The PR was fixed with patch v2, since the initial as well as resulting > insns make use of hard register constraints. > > I only did a quick test of v2 and it worked. It is getting late here > and I will come back to this tomorrow. Just wanted to give an update.
Giving this a second thought, I still believe that this is the right fix. Patch v2 was not wrong but it didn't catch the real problem. That was probably what's been bothering me the whole time. One might even check whether the initial insn into which a combination is about to happen, had the same hard register constraints as the resulting one, and allow a combination in this case. Since this would have meant that prior combine, a hard register constraint must have been satisfiable (or we had a problem before combine), and that in turn means that after a combination, the hard register constraints would still be satisfiable. Anyhow, this patch is about fixing errors/ICEs and not about missed-optimizations and I would like to keep it simple while in stage 4. Bootstrap and regtest are successful on s390x and x86_64. Tested PR via a cross successfully. Ok for mainline? Cheers, Stefan > > I have seen that target i386 uses extract_insn() in > TARGET_LEGITIMATE_COMBINED_INSN which is called directly after my > changes. Instead of calling it twice in a row for the same insn, one > could establish a contract for TARGET_LEGITIMATE_COMBINED_INSN where > targets may assume that the insn has been already extracted. Maybe > something for stage 1. > > -- >8 -- > > From: Stefan Schulze Frielinghaus <[email protected]> > > This fixes > > t.c:6:1: error: unable to find a register to spill > 6 | } > | ^ > > for target avr. In the PR we are given a patch which makes use of hard > register constraints in the machine description for divmodhi4. Prior > combine we have for the test from the PR > > (insn 7 6 8 2 (parallel [ > (set (reg:HI 46 [ _1 ]) > (div:HI (reg/v:HI 44 [ k ]) > (reg:HI 48))) > (set (reg:HI 47) > (mod:HI (reg/v:HI 44 [ k ]) > (reg:HI 48))) > (clobber (scratch:HI)) > (clobber (scratch:QI)) > ]) "t.c":5:5 602 {divmodhi4} > (expr_list:REG_DEAD (reg:HI 48) > (expr_list:REG_DEAD (reg/v:HI 44 [ k ]) > (expr_list:REG_UNUSED (reg:HI 47) > (nil))))) > (insn 8 7 9 2 (set (reg:HI 22 r22) > (symbol_ref/f:HI ("*.LC0") [flags 0x2] <var_decl 0x3fff7950d10 > *.LC0>)) "t.c":5:5 128 {*movhi_split} > (nil)) > (insn 9 8 10 2 (set (reg:HI 24 r24) > (reg:HI 46 [ _1 ])) "t.c":5:5 128 {*movhi_split} > (expr_list:REG_DEAD (reg:HI 46 [ _1 ]) > (nil))) > > The patched instruction divmodhi4 constraints operand 2 (here pseudo > 48) to hard register 22. Combine merges insn 7 into 9 by crossing a > hard register assignment of register 22. > > (note 7 6 8 2 NOTE_INSN_DELETED) > (insn 8 7 9 2 (set (reg:HI 22 r22) > (symbol_ref/f:HI ("*.LC0") [flags 0x2] <var_decl 0x3fff7950d10 > *.LC0>)) "t.c":5:5 128 {*movhi_split} > (nil)) > (insn 9 8 10 2 (parallel [ > (set (reg:HI 24 r24) > (div:HI (reg:HI 49 [ k ]) > (reg:HI 48))) > (set (reg:HI 47) > (mod:HI (reg:HI 49 [ k ]) > (reg:HI 48))) > (clobber (scratch:HI)) > (clobber (scratch:QI)) > ]) "t.c":5:5 602 {divmodhi4} > (expr_list:REG_DEAD (reg:HI 48) > (expr_list:REG_DEAD (reg:HI 49 [ k ]) > (nil)))) > > This leaves us with a conflict for pseudo 48 in the updated insn 9 since > register 22 is live here. > > Fixed by pulling the sledge hammer and rejecting any resulting insn > which makes use of hard register constraints. Ideally we would skip > based on the fact whether a combination crosses a hard register > assignment and the corresponding hard register is also referred by a > single register constraint of the resulting insn. > > gcc/ChangeLog: > > * combine.cc (recog_for_combine_1): Reject insns which make use > of hard register constraints. > --- > gcc/combine.cc | 38 ++++++++++++++++++++++++++++++++++---- > 1 file changed, 34 insertions(+), 4 deletions(-) > > diff --git a/gcc/combine.cc b/gcc/combine.cc > index 09e24347b34..fa8435ffd11 100644 > --- a/gcc/combine.cc > +++ b/gcc/combine.cc > @@ -11570,12 +11570,42 @@ recog_for_combine_1 (rtx *pnewpat, rtx_insn *insn, > rtx *pnotes, > REG_NOTES (insn) = notes; > INSN_CODE (insn) = insn_code_number; > > - /* Allow targets to reject combined insn. */ > - if (!targetm.legitimate_combined_insn (insn)) > + /* Do not accept an insn if hard register constraints are used. For > + example, assume that the first insn is combined into the last one: > + > + r100=... > + %5=... > + r101=exp(r100) > + > + If the resulting insn has an operand which is constrained to hard > + register %5, then this introduces a conflict since register %5 is live > + at this point. Therefore, skip for now. This is a sledge hammer > + approach. Ideally we would skip based on the fact whether a > + combination crosses a hard register assignment and the corresponding > + hard register is also referred by a single register constraint of the > + resulting insn. */ > + bool has_hard_reg_cstr = false; > + extract_insn (insn); > + for (int nop = recog_data.n_operands - 1; nop >= 0; --nop) > + if (strchr (recog_data.constraints[nop], '{')) > + { > + has_hard_reg_cstr = true; > + break; > + } > + > + /* Don't accept hard register constraints. Allow targets to reject > + combined insn. */ > + if (has_hard_reg_cstr || !targetm.legitimate_combined_insn (insn)) > { > if (dump_file && (dump_flags & TDF_DETAILS)) > - fputs ("Instruction not appropriate for target.", > - dump_file); > + { > + if (has_hard_reg_cstr) > + fputs ("Instruction makes use of hard register constraints.", > + dump_file); > + else > + fputs ("Instruction not appropriate for target.", > + dump_file); > + } > > /* Callers expect recog_for_combine to strip > clobbers from the pattern on failure. */ > -- > 2.52.0 >
