Re: [PATCH v2] combine: Fix ICE in try_combine on pr112494.c [PR112560]

[Uros Bizjak <ubizjak@gmail.com>]

On Mon, Mar 18, 2024 at 3:46 PM Segher Boessenkool
<segher@kernel.crashing.org> wrote:
>
> On Thu, Mar 07, 2024 at 11:27:28PM +0100, Uros Bizjak wrote:
> > On Thu, Mar 7, 2024 at 11:07 PM Uros Bizjak <ubizjak@gmail.com> wrote:
> > > > >  (unspec:DI [
> > > > >          (reg:CC 17 flags)
> > > > >      ] UNSPEC_PUSHFL)
> > > >
> > > > But that is invalid RTL?  The only valid use of a CC is written as
> > > > cc-compared-to-0.  It means "the result of something compared to 0,
> > > > stored in that cc reg".
> > > >
> > > > (And you can copy a CC reg around, but that is not a use ;-) )
> >
> > Hm... under this premise, we can also say that any form that is not
> > cc-compared-to-0 is not a use.
>
> Well, no.  All uses of CC are written as comparisons to 0, or are pure
> dataflow.  Anything else is not "not a use" but just invalid.
>
> > Consequently, if it is not a use, then
> > the CC reg should not be updated at its use location, so my v1 patch,
> > where we simply skip the update (but retain the combined insn),
> > actually makes sense.
>
> With more asserts, perhaps.
>
> > In this concrete situation, we don't care about CC register mode in
> > the PUSHFL insn. And we should not change CC reg mode of the use,
> > because any other mode than the generic CCmode won't be recognized by
> > the insn pattern.
>
> You always care about the CC mode, that is why you always write it as
> comparison, so the backend can choose a mode based on what the flag bits
> mean in this context.  For an extreme example look at 390, but on pretty
> much any target both signed and unsigned comparisons use the same flag
> bits, and maybe fp comparisons as well.

After some more thoughts, I came up with v3 [1], where the mode is
updated also in non-comparison use. But instead of a blind SUBST, we
find the correct use location and update the mode accordingly. If the
new mode is not recognized in the use insn, then the whole combination
is cancelled. Since x86 pushfl does not care about CCmode, I have
changed it to handle all CCmodes. Please note, that with this
approach, the backend can choose a mode.

[1] https://gcc.gnu.org/pipermail/gcc-patches/2024-March/647634.html

> But pushfl does sound like pure dataflow.  Why is this a builtin, is
> it ever a good idea if the user writes stuff the compiler can do a
> better job doing itself, not to mention it is way easier for the
> compiler anyway?  :-)

The mode of the pushfl has to be correct, because it operates on
stack. Unfortunately, the plain move can't do this due to
WORDmode/CCmode mismatch in the rtx, so UNSPEC is necessary. But it IS
a pure dataflow instruction - it is a PUSH.

Uros.