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

[Segher Boessenkool <segher@kernel.crashing.org>]

On Thu, Mar 07, 2024 at 11:07:18PM +0100, Uros Bizjak wrote:
> On Thu, Mar 7, 2024 at 10:37 PM Segher Boessenkool
> <segher@kernel.crashing.org> wrote:
> > > but can be something else, such as the above noted
> > >
> > >  (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 ;-) )
> 
> How can we describe a pushfl then?

(unspec:DI [
        (compare:CC) ((reg:CC 17 flags) (const_int 0))
    ] UNSPEC_PUSHFL)

or something like that?

> It was changed to its current form
> in [1], but I suspect that the code will try to update it even when
> pushfl is implemented as a direct move from a register (as was defined
> previously).
> 
> BTW: Unspecs are used in a couple of other places for other targets [2].
> 
> [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=112494#c5
> [2] https://gcc.gnu.org/pipermail/gcc-patches/2023-December/639743.html

There is nothing wront with unspecs.  You cannot use a CCmode value
without comparing it to 0, though.  The exact kind of comparison
determines what bits are valid (and have what meaning) in your CC reg,
even!

> > > The source code that deals with the *user* of the CC register assumes
> > > the former form, so it blindly tries to update the mode of the CC
> > > register inside LT comparison RTX
> >
> > Would you like it better if there was an assert for this?  There are
> > very many RTL requirements that aren't chacked for currently :-/
> 
> In this case - yes. Assert signals that something is unsupported (or
> invalid), way better than silently corrupting some memory, reporting
> the corruption only with checking enabled.

Yeah.  The way RTL checking works makes this hard to do in most cases.
Hrm.  (It cannot easily look at context, only inside of the current RTX).

> > The unspec should have the CC compared with 0 as argument.
> 
> But this does not do what pushfl does... It pushes the register to the stack.

Can't you just describe the dataflow then, without an unspec?  An unspec
by definition does some (unspecified) operation on the data.


Segher