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
