Re: [RFA] [PR rtl-optimization/90275] Handle nop reg->reg copies in cse

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

Hi all,

On Wed, Feb 05, 2020 at 07:26:03AM +0100, Jakub Jelinek wrote:
> On Tue, Feb 04, 2020 at 06:04:09PM -0700, Jeff Law wrote:
> > --- a/gcc/cse.c
> > +++ b/gcc/cse.c
> > @@ -5572,6 +5572,16 @@ cse_insn (rtx_insn *insn)
> >  	  sets[i].rtl = 0;
> >  	}
> >  
> > +      /* Similarly for no-op moves.  */

It says "no-op MEM moves" right above, so it should say "no-op REG
moves" here?

> > +      if (n_sets == 1
> > +	  && GET_CODE (src) == REG
> 
> Just nits:
> REG_P (src) ?

Hey that is my nit!  Find your own!  ;-)

> > +	  && src == dest)
> 
> Is pointer comparison ok?

It isn't, it doesn't work for hard registers.

> I mean, shouldn't we instead do
> rtx_equal_p (src, dest),

This does not see e.g.
  (set (reg:SI 123) (subreg:SI (reg:DI 123) 0))
as no-op move.

> set_noop_p (sets[i].rtl)

This doesn't catch all such cases either.

> or noop_move_p (insn)?

And this one is plain wrong (it should be called something with "maybe"
in the name, it returns false if it thinks that may lead to better
optimisation, see the REG_EQUAL handling).

What we need here is a test whether CSE can ignore this insn, and we
will run into this problem if we don't (Jeff's analysis).  Does CSE
already have everything it uses to make that decision scribbled away
somewhere, when we get here?  It would be good if we could use the
exact same condition (same predicate function for example) as what
would lead to the problem later, or we'll be playing whack-a-mole for
a while (or CSE is completely rewritten soon, my preferred option, but
"soon" on a GCC timescale will take way too long for the PR).


Segher