From: Richard Biener <rguenther@suse.de>
To: Uros Bizjak <ubizjak@gmail.com>
Cc: "gcc-patches@gcc.gnu.org" <gcc-patches@gcc.gnu.org>,
Jeff Law <jeffreyalaw@gmail.com>,
segher@kernel.crashing.org
Subject: Re: [PATCH v2] combine: Fix ICE in try_combine on pr112494.c [PR112560]
Date: Thu, 7 Mar 2024 11:22:12 +0100 (CET) [thread overview]
Message-ID: <3n4484r9-189o-rro2-20q6-p839o6575247@fhfr.qr> (raw)
In-Reply-To: <CAFULd4bTfvUOZ8MvapkOoJWoeXr95UJ=ggeoufaCyi40ZA8JPg@mail.gmail.com>
On Thu, 7 Mar 2024, Uros Bizjak wrote:
> On Thu, Mar 7, 2024 at 10:56?AM Richard Biener <rguenther@suse.de> wrote:
> >
> > On Thu, 7 Mar 2024, Uros Bizjak 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.
> > >
> > > Undo the combination if *cc_use_loc is not COMPARISON_P.
> > >
> > > Also remove buggy and now redundant check for (const 0) RTX as part of
> > > the comparison.
> > >
> > > PR rtl-optimization/112560
> > >
> > > gcc/ChangeLog:
> > >
> > > * combine.cc (try_combine): Reject the combination
> > > if *cc_use_loc is not COMPARISON_P.
> > >
> > > Bootstrapped and regression tested on x86_64-pc-linux-gnu {,-m32}.
> > >
> > > OK for trunk?
> >
> > Since you CCed me - looking at the code I wonder why we fatally fail.
> > The following might also fix the issue and preserve more of the
> > rest of the flow of the function.
> >
> > If that works I'd prefer it. But I'll defer approval to the combine
> > maintainer which is Segher.
>
> Your patch is basically what v1 did [1], but it was suggested (in a
> reply by you ;) ) that we should stop the attempt to combine if we
> can't handle the use. So, the v2 patch undoes the combine and records
> a nice message in this case.
Ah, sorry. I think I was merely curious how this works at all.
Your patch doesn't stop when find_single_use returns NULL
for multiple uses, but that case (as compared to "no other use")
should be handled the same as a !COMPARISON_P single-use. The
original patch preserves the multi-use behavior which might be
a lot more unlikely than the !COMPARISON_P use case.
That said, in the original mail I questioned whether combine
handles the multi-use case correctly at all. Looking at
find_single_use it seems to implement find_first_use.
Or get_use_of_single_use with the pretext that there actually
is only at most a single use (it doesn't seem to assert there
is one?). Strange function, that.
But I don't know combine, I hope Segher can answer and pick a
patch.
Richard.
> Also, please note the removal of an existing crude hack that tries to
> reject non-comparison uses by looking for (const_int 0) in the use
> RTX, which is wrong as well.
>
> [1] https://gcc.gnu.org/pipermail/gcc-patches/2023-November/638589.html
>
> Uros.
>
> >
> > Thanks,
> > Richard.
> >
> > diff --git a/gcc/combine.cc b/gcc/combine.cc
> > index a4479f8d836..e280cd72ec7 100644
> > --- a/gcc/combine.cc
> > +++ b/gcc/combine.cc
> > @@ -3182,7 +3182,8 @@ try_combine (rtx_insn *i3, rtx_insn *i2, rtx_insn
> > *i1, rtx_insn *i0,
> >
> > if (undobuf.other_insn == 0
> > && (cc_use_loc = find_single_use (SET_DEST (newpat), i3,
> > - &cc_use_insn)))
> > + &cc_use_insn))
> > + && 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))
> > @@ -3200,7 +3201,7 @@ try_combine (rtx_insn *i3, rtx_insn *i2, rtx_insn
> > *i1, rtx_insn *i0,
> > the above simplify_compare_const() returned a new comparison
> > operator. undobuf.other_insn is assigned the CC use insn
> > when modifying it. */
> > - if (cc_use_loc)
> > + if (cc_use_loc && COMPARISON_P (*cc_use_loc))
> > {
> > #ifdef SELECT_CC_MODE
> > machine_mode new_mode
> >
>
--
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH,
Frankenstrasse 146, 90461 Nuernberg, Germany;
GF: Ivo Totev, Andrew McDonald, Werner Knoblich; (HRB 36809, AG Nuernberg)
next prev parent reply other threads:[~2024-03-07 11:06 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-07 9:16 Uros Bizjak
2024-03-07 9:55 ` Richard Biener
2024-03-07 10:11 ` Uros Bizjak
2024-03-07 10:22 ` Richard Biener [this message]
2024-03-07 17:44 ` Segher Boessenkool
2024-03-07 10:37 ` Jakub Jelinek
2024-03-07 10:45 ` Richard Biener
2024-03-07 11:22 ` Uros Bizjak
2024-03-07 17:51 ` Segher Boessenkool
2024-03-07 17:49 ` Segher Boessenkool
2024-03-07 10:57 ` Uros Bizjak
2024-03-07 17:36 ` Segher Boessenkool
2024-03-07 21:04 ` Uros Bizjak
2024-03-07 21:08 ` Uros Bizjak
2024-03-07 21:35 ` Segher Boessenkool
2024-03-07 22:07 ` Uros Bizjak
2024-03-07 22:27 ` Segher Boessenkool
2024-03-07 22:46 ` Uros Bizjak
2024-03-18 14:49 ` Segher Boessenkool
2024-03-18 15:44 ` Uros Bizjak
2024-03-07 22:27 ` Uros Bizjak
2024-03-18 14:44 ` Segher Boessenkool
2024-03-18 15:29 ` Uros Bizjak
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=3n4484r9-189o-rro2-20q6-p839o6575247@fhfr.qr \
--to=rguenther@suse.de \
--cc=gcc-patches@gcc.gnu.org \
--cc=jeffreyalaw@gmail.com \
--cc=segher@kernel.crashing.org \
--cc=ubizjak@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).