| Message ID | CAFULd4Zboqhmv=BUHoKJfRaT9YfNbg5OwsPuUsMyW-dbsr2M5g@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | combine: Fix ICE in try_combine on pr112494.c [PR112560] | expand |
On Wed, Nov 29, 2023 at 10:35 AM Uros Bizjak <ubizjak@gmail.com> wrote: > > The compiler, configured with --enable-checking=yes,rtl,extra ICEs with: > > internal compiler error: RTL check: expected elt 0 type 'e' or 'u', > have 'E' (rtx unspec) in try_combine, at combine.cc:3237 > > This is > > 3236 /* Just replace the CC reg with a new mode. */ > 3237 SUBST (XEXP (*cc_use_loc, 0), newpat_dest); > 3238 undobuf.other_insn = cc_use_insn; > > in combine.cc, where *cc_use_loc is > > (unspec:DI [ > (reg:CC 17 flags) > ] UNSPEC_PUSHFL) > > combine assumes CC must be used inside of a comparison and uses XEXP (..., 0) > without checking on the RTX type of the argument. > > Skip the modification of CC-using operation if *cc_use_loc is not COMPARISON_P. > > PR middle-end/112560 > > gcc/ChangeLog: > > * combine.cc (try_combine): Skip the modification of CC-using > operation if *cc_use_loc is not COMPARISON_P. > > Bootstrapped and regression tested on x86_64-linux-gnu {,-m32}. > > OK for master? Don't we need to stop the attempt to combine when we cannot handle a use? Simply not adjusting another use doesn't look correct, does it? Richard. > > Uros.
On Wed, Nov 29, 2023 at 1:25 PM Richard Biener <richard.guenther@gmail.com> wrote: > > On Wed, Nov 29, 2023 at 10:35 AM Uros Bizjak <ubizjak@gmail.com> wrote: > > > > The compiler, configured with --enable-checking=yes,rtl,extra ICEs with: > > > > internal compiler error: RTL check: expected elt 0 type 'e' or 'u', > > have 'E' (rtx unspec) in try_combine, at combine.cc:3237 > > > > This is > > > > 3236 /* Just replace the CC reg with a new mode. */ > > 3237 SUBST (XEXP (*cc_use_loc, 0), newpat_dest); > > 3238 undobuf.other_insn = cc_use_insn; > > > > in combine.cc, where *cc_use_loc is > > > > (unspec:DI [ > > (reg:CC 17 flags) > > ] UNSPEC_PUSHFL) > > > > combine assumes CC must be used inside of a comparison and uses XEXP (..., 0) > > without checking on the RTX type of the argument. > > > > Skip the modification of CC-using operation if *cc_use_loc is not COMPARISON_P. > > > > PR middle-end/112560 > > > > gcc/ChangeLog: > > > > * combine.cc (try_combine): Skip the modification of CC-using > > operation if *cc_use_loc is not COMPARISON_P. > > > > Bootstrapped and regression tested on x86_64-linux-gnu {,-m32}. > > > > OK for master? > > Don't we need to stop the attempt to combine when we cannot handle a use? > Simply not adjusting another use doesn't look correct, does it? I was assuming that if the CC reg is not used inside the comparison, then the mode of CC reg is irrelevant. We can still combine the instructions into new insn, without updating the use of CC reg. In this particular case, the combined insn is rejected, but UNSPEC_PUSHFL does not care about the mode of the CC reg and would handle combined insn just fine. Alternatively, the attached patch skips the combination altogether. Thanks, Uros. diff --git a/gcc/combine.cc b/gcc/combine.cc index 6344cd3c9f2..e533631d0e6 100644 --- a/gcc/combine.cc +++ b/gcc/combine.cc @@ -3184,11 +3184,21 @@ try_combine (rtx_insn *i3, rtx_insn *i2, rtx_insn *i1, rtx_insn *i0, && (cc_use_loc = find_single_use (SET_DEST (newpat), i3, &cc_use_insn))) { - compare_code = orig_compare_code = GET_CODE (*cc_use_loc); - if (is_a <scalar_int_mode> (GET_MODE (i2dest), &mode)) - compare_code = simplify_compare_const (compare_code, mode, - &op0, &op1); - target_canonicalize_comparison (&compare_code, &op0, &op1, 1); + if (COMPARISON_P (*cc_use_loc)) + { + compare_code = orig_compare_code = GET_CODE (*cc_use_loc); + if (is_a <scalar_int_mode> (GET_MODE (i2dest), &mode)) + compare_code = simplify_compare_const (compare_code, mode, + &op0, &op1); + target_canonicalize_comparison (&compare_code, &op0, &op1, 1); + } + else + { + if (dump_file && (dump_flags & TDF_DETAILS)) + fprintf (dump_file, "CC register not used in comparison.\n"); + undo_all (); + return 0; + } } /* Do the rest only if op1 is const0_rtx, which may be the
Hi! On Wed, Nov 29, 2023 at 02:20:03PM +0100, Uros Bizjak wrote: > On Wed, Nov 29, 2023 at 1:25 PM Richard Biener > <richard.guenther@gmail.com> wrote: > > On Wed, Nov 29, 2023 at 10:35 AM Uros Bizjak <ubizjak@gmail.com> wrote: > I was assuming that if the CC reg is not used inside the comparison, > then the mode of CC reg is irrelevant. We can still combine the > instructions into new insn, without updating the use of CC reg. It should never happen that the CC reg is not used, so what does this mean? Where it is used might not be a comparison of course, as in your example. > && (cc_use_loc = find_single_use (SET_DEST (newpat), i3, > &cc_use_insn))) > { > - compare_code = orig_compare_code = GET_CODE (*cc_use_loc); > - if (is_a <scalar_int_mode> (GET_MODE (i2dest), &mode)) > - compare_code = simplify_compare_const (compare_code, mode, > - &op0, &op1); > - target_canonicalize_comparison (&compare_code, &op0, &op1, 1); > + if (COMPARISON_P (*cc_use_loc)) > + { > + compare_code = orig_compare_code = GET_CODE (*cc_use_loc); > + if (is_a <scalar_int_mode> (GET_MODE (i2dest), &mode)) > + compare_code = simplify_compare_const (compare_code, mode, > + &op0, &op1); > + target_canonicalize_comparison (&compare_code, &op0, &op1, 1); > + } > + else > + { > + if (dump_file && (dump_flags & TDF_DETAILS)) > + fprintf (dump_file, "CC register not used in comparison.\n"); "Where the CCmode register is used is not a comparison". But more compact if you can manage (I cannot). Segher
On Thu, Nov 30, 2023 at 9:21 AM Segher Boessenkool <segher@kernel.crashing.org> wrote: > > Hi! > > On Wed, Nov 29, 2023 at 02:20:03PM +0100, Uros Bizjak wrote: > > On Wed, Nov 29, 2023 at 1:25 PM Richard Biener > > <richard.guenther@gmail.com> wrote: > > > On Wed, Nov 29, 2023 at 10:35 AM Uros Bizjak <ubizjak@gmail.com> wrote: > > I was assuming that if the CC reg is not used inside the comparison, > > then the mode of CC reg is irrelevant. We can still combine the > > instructions into new insn, without updating the use of CC reg. > > It should never happen that the CC reg is not used, so what does this > mean? I was trying to say that when CC reg is used inside the comparison, e.g.: (define_insn "*setcc_qi" [(set (match_operand:QI 0 "nonimmediate_operand" "=qm") (match_operator:QI 1 "ix86_comparison_operator" [(reg FLAGS_REG) (const_int 0)]))] "" "set%C1\t%0" and through the%C operand modifier we call put_condition_code, the mode of CC reg is checked and a different instruction is emitted, depending on the CC reg mode. Please also note that not all comparison operators are handled, so when the combine rewrites the comparison, it isn't necessarily supported. This is in contrast with: (define_insn "@pushfl<mode>2" [(set (match_operand:W 0 "push_operand" "=<") (unspec:W [(match_operand:CC 1 "flags_reg_operand")] UNSPEC_PUSHFL))] where we don't care about CC reg mode at all. The CC reg is not in the comparison, so the mode does not matter. The combination of instructions should not be limited by CC reg user in this case. > > Where it is used might not be a comparison of course, as in your > example. > > > && (cc_use_loc = find_single_use (SET_DEST (newpat), i3, > > &cc_use_insn))) > > { > > - compare_code = orig_compare_code = GET_CODE (*cc_use_loc); > > - if (is_a <scalar_int_mode> (GET_MODE (i2dest), &mode)) > > - compare_code = simplify_compare_const (compare_code, mode, > > - &op0, &op1); > > - target_canonicalize_comparison (&compare_code, &op0, &op1, 1); > > + if (COMPARISON_P (*cc_use_loc)) > > + { > > + compare_code = orig_compare_code = GET_CODE (*cc_use_loc); > > + if (is_a <scalar_int_mode> (GET_MODE (i2dest), &mode)) > > + compare_code = simplify_compare_const (compare_code, mode, > > + &op0, &op1); > > + target_canonicalize_comparison (&compare_code, &op0, &op1, 1); > > + } > > + else > > + { > > + if (dump_file && (dump_flags & TDF_DETAILS)) > > + fprintf (dump_file, "CC register not used in comparison.\n"); > > "Where the CCmode register is used is not a comparison". But more > compact if you can manage (I cannot). Perhaps "CCmode register user is not a comparison." ? Uros.
On Wed, Nov 29, 2023 at 1:25 PM Richard Biener <richard.guenther@gmail.com> wrote: > > On Wed, Nov 29, 2023 at 10:35 AM Uros Bizjak <ubizjak@gmail.com> wrote: > > > > The compiler, configured with --enable-checking=yes,rtl,extra ICEs with: > > > > internal compiler error: RTL check: expected elt 0 type 'e' or 'u', > > have 'E' (rtx unspec) in try_combine, at combine.cc:3237 > > > > This is > > > > 3236 /* Just replace the CC reg with a new mode. */ > > 3237 SUBST (XEXP (*cc_use_loc, 0), newpat_dest); > > 3238 undobuf.other_insn = cc_use_insn; > > > > in combine.cc, where *cc_use_loc is > > > > (unspec:DI [ > > (reg:CC 17 flags) > > ] UNSPEC_PUSHFL) > > > > combine assumes CC must be used inside of a comparison and uses XEXP (..., 0) > > without checking on the RTX type of the argument. > > > > Skip the modification of CC-using operation if *cc_use_loc is not COMPARISON_P. > > > > PR middle-end/112560 > > > > gcc/ChangeLog: > > > > * combine.cc (try_combine): Skip the modification of CC-using > > operation if *cc_use_loc is not COMPARISON_P. > > > > Bootstrapped and regression tested on x86_64-linux-gnu {,-m32}. > > > > OK for master? > > Don't we need to stop the attempt to combine when we cannot handle a use? > Simply not adjusting another use doesn't look correct, does it? I have analysed [1] all targets that define SELECT_CC_MODE, and almost all use CC_REG exclusively in comparison. Besides i386 that defines: (define_insn "@pushfl<mode>2" [(set (match_operand:W 0 "push_operand" "=<") (unspec:W [(match_operand:CC 1 "flags_reg_operand")] UNSPEC_PUSHFL))] other non-comparison pre-reload uses of CC_REG are: arm: (define_insn "get_fpscr_nzcvqc" [(set (match_operand:SI 0 "register_operand" "=r") (unspec_volatile:SI [(reg:SI VFPCC_REGNUM)] UNSPEC_GET_FPSCR_NZCVQC))] (define_insn "mve_vadcq_<supf>v4si" [(set (match_operand:V4SI 0 "s_register_operand" "=w") (unspec:V4SI [(match_operand:V4SI 1 "s_register_operand" "w") (match_operand:V4SI 2 "s_register_operand" "w")] VADCQ)) (set (reg:SI VFPCC_REGNUM) (unspec:SI [(reg:SI VFPCC_REGNUM)] VADCQ)) ] rs6000: (define_insn "prologue_movesi_from_cr" [(set (match_operand:SI 0 "gpc_reg_operand" "=r") (unspec:SI [(reg:CC CR2_REGNO) (reg:CC CR3_REGNO) (reg:CC CR4_REGNO)] UNSPEC_MOVESI_FROM_CR))] and just for reference s390: (define_insn_and_split "*ccraw_to_int" [(set (pc) (if_then_else (match_operator 0 "s390_eqne_operator" [(reg:CCRAW CC_REGNUM) (match_operand 1 "const_int_operand" "")]) (label_ref (match_operand 2 "" "")) (pc))) (set (match_operand:SI 3 "register_operand" "=d") (unspec:SI [(reg:CCRAW CC_REGNUM)] UNSPEC_CC_TO_INT))] The above is not single_use, so the issue does not apply here. These uses can all break with checking enabled at the mentioned spot in combine.cc in the same way as x86. Actually, it is undesirable to change the mode in the "other instruction" - the machine instruction doesn't care about mode at all, but the insn pattern may fail recognition due to CC mode change. Based on the above analysis, I propose we proceed with my original patch. [1] Starting with 'egrep -v -w "set|clobber" *.md | grep <CC_REG>' and analysing all hits Uros.
On Mon, Dec 4, 2023 at 10:34 AM Uros Bizjak <ubizjak@gmail.com> wrote: > > On Wed, Nov 29, 2023 at 1:25 PM Richard Biener > <richard.guenther@gmail.com> wrote: > > > > On Wed, Nov 29, 2023 at 10:35 AM Uros Bizjak <ubizjak@gmail.com> wrote: > > > > > > The compiler, configured with --enable-checking=yes,rtl,extra ICEs with: > > > > > > internal compiler error: RTL check: expected elt 0 type 'e' or 'u', > > > have 'E' (rtx unspec) in try_combine, at combine.cc:3237 > > > > > > This is > > > > > > 3236 /* Just replace the CC reg with a new mode. */ > > > 3237 SUBST (XEXP (*cc_use_loc, 0), newpat_dest); > > > 3238 undobuf.other_insn = cc_use_insn; > > > > > > in combine.cc, where *cc_use_loc is > > > > > > (unspec:DI [ > > > (reg:CC 17 flags) > > > ] UNSPEC_PUSHFL) > > > > > > combine assumes CC must be used inside of a comparison and uses XEXP (..., 0) > > > without checking on the RTX type of the argument. > > > > > > Skip the modification of CC-using operation if *cc_use_loc is not COMPARISON_P. > > > > > > PR middle-end/112560 > > > > > > gcc/ChangeLog: > > > > > > * combine.cc (try_combine): Skip the modification of CC-using > > > operation if *cc_use_loc is not COMPARISON_P. > > > > > > Bootstrapped and regression tested on x86_64-linux-gnu {,-m32}. > > > > > > OK for master? > > > > Don't we need to stop the attempt to combine when we cannot handle a use? > > Simply not adjusting another use doesn't look correct, does it? > > I have analysed [1] all targets that define SELECT_CC_MODE, and almost > all use CC_REG exclusively in comparison. Besides i386 that defines: > > (define_insn "@pushfl<mode>2" > [(set (match_operand:W 0 "push_operand" "=<") > (unspec:W [(match_operand:CC 1 "flags_reg_operand")] > UNSPEC_PUSHFL))] > > other non-comparison pre-reload uses of CC_REG are: > > arm: > > (define_insn "get_fpscr_nzcvqc" > [(set (match_operand:SI 0 "register_operand" "=r") > (unspec_volatile:SI [(reg:SI VFPCC_REGNUM)] UNSPEC_GET_FPSCR_NZCVQC))] > > (define_insn "mve_vadcq_<supf>v4si" > [(set (match_operand:V4SI 0 "s_register_operand" "=w") > (unspec:V4SI [(match_operand:V4SI 1 "s_register_operand" "w") > (match_operand:V4SI 2 "s_register_operand" "w")] > VADCQ)) > (set (reg:SI VFPCC_REGNUM) > (unspec:SI [(reg:SI VFPCC_REGNUM)] > VADCQ)) > ] > > rs6000: > > (define_insn "prologue_movesi_from_cr" > [(set (match_operand:SI 0 "gpc_reg_operand" "=r") > (unspec:SI [(reg:CC CR2_REGNO) (reg:CC CR3_REGNO) > (reg:CC CR4_REGNO)] > UNSPEC_MOVESI_FROM_CR))] > > and just for reference s390: > > (define_insn_and_split "*ccraw_to_int" > [(set (pc) > (if_then_else > (match_operator 0 "s390_eqne_operator" > [(reg:CCRAW CC_REGNUM) > (match_operand 1 "const_int_operand" "")]) > (label_ref (match_operand 2 "" "")) > (pc))) > (set (match_operand:SI 3 "register_operand" "=d") > (unspec:SI [(reg:CCRAW CC_REGNUM)] UNSPEC_CC_TO_INT))] > > The above is not single_use, so the issue does not apply here. > > These uses can all break with checking enabled at the mentioned spot > in combine.cc in the same way as x86. Actually, it is undesirable to > change the mode in the "other instruction" - the machine instruction > doesn't care about mode at all, but the insn pattern may fail > recognition due to CC mode change. > > Based on the above analysis, I propose we proceed with my original patch. I'm not familiar enough with combine nor CC reg uses to approve the patch (I just wanted to point out what I noticed when browsing patches). I'll defer to Segher for approval. Thanks, Richard. > [1] Starting with 'egrep -v -w "set|clobber" *.md | grep <CC_REG>' and > analysing all hits > > Uros.
diff --git a/gcc/combine.cc b/gcc/combine.cc index 6344cd3c9f2..a270138b59f 100644 --- a/gcc/combine.cc +++ b/gcc/combine.cc @@ -3184,11 +3184,16 @@ try_combine (rtx_insn *i3, rtx_insn *i2, rtx_insn *i1, rtx_insn *i0, && (cc_use_loc = find_single_use (SET_DEST (newpat), i3, &cc_use_insn))) { - compare_code = orig_compare_code = GET_CODE (*cc_use_loc); - if (is_a <scalar_int_mode> (GET_MODE (i2dest), &mode)) - compare_code = simplify_compare_const (compare_code, mode, - &op0, &op1); - target_canonicalize_comparison (&compare_code, &op0, &op1, 1); + if (COMPARISON_P (*cc_use_loc)) + { + compare_code = orig_compare_code = GET_CODE (*cc_use_loc); + if (is_a <scalar_int_mode> (GET_MODE (i2dest), &mode)) + compare_code = simplify_compare_const (compare_code, mode, + &op0, &op1); + target_canonicalize_comparison (&compare_code, &op0, &op1, 1); + } + else + cc_use_loc = NULL; } /* Do the rest only if op1 is const0_rtx, which may be the
The compiler, configured with --enable-checking=yes,rtl,extra ICEs with: internal compiler error: RTL check: expected elt 0 type 'e' or 'u', have 'E' (rtx unspec) in try_combine, at combine.cc:3237 This is 3236 /* Just replace the CC reg with a new mode. */ 3237 SUBST (XEXP (*cc_use_loc, 0), newpat_dest); 3238 undobuf.other_insn = cc_use_insn; in combine.cc, where *cc_use_loc is (unspec:DI [ (reg:CC 17 flags) ] UNSPEC_PUSHFL) combine assumes CC must be used inside of a comparison and uses XEXP (..., 0) without checking on the RTX type of the argument. Skip the modification of CC-using operation if *cc_use_loc is not COMPARISON_P. PR middle-end/112560 gcc/ChangeLog: * combine.cc (try_combine): Skip the modification of CC-using operation if *cc_use_loc is not COMPARISON_P. Bootstrapped and regression tested on x86_64-linux-gnu {,-m32}. OK for master? Uros.