Re: [PATCH] Fix combiner (PRs rtl-optimization/46034, rtl-optimization/46212, rtl-optimization/46248)

[Jakub Jelinek <jakub@redhat.com>]

On Tue, Nov 02, 2010 at 09:27:29PM +0100, Eric Botcazou wrote:
> > There are two issues, one is that the earlier
> > newpat = subst (newpat, i0dest, i0src, ...);
> > might have (but not necessarily) have changed i1src and so when i1dest
> > is first replaced with i1src that way modified and then i0dest is replaced
> > with i0src, the replacements are already wrong and as testcases show
> > self-referential.
> 
> FWIW I also debugged this (and spotted a pasto in the 4-insn combiner patch 
> that I'll fix after your fixes, patch attached).  I also roughly came up with:
> 
> +      /* Following subst may modify i1src, make a copy of it
> +        before it is for added_sets_2 handling if needed.  */
> +      if (added_sets_2
> +         && i0dest_in_i0src
> +         && i0_feeds_i1_n
> +         && (i1_feeds_i2_n || i0_feeds_i2_n))
> +       i1src_copy = copy_rtx (i1src);
> 
> but why not just
> 
>  if (i0_feeds_i1_n && added_sets_2 && i1_feeds_i2_n)
> 
> i.e. make a copy if substituting I0 will clobber I1SRC and I1SRC will be re- 
> substituted in I2PAT?

I think you are right, if !i1_feeds_i2_n then the copy is not needed,
because it will not be used.  If !i0dest_in_i0src, I think
the copy is not strictily needed, because it shouldn't matter whether
i0dest is replaced with i0src just once or more than once, but if you
prefer the if (i0_feeds_i1_n && added_sets_2 && i1_feeds_i2_n)
condition, I can bootstrap/regtest it with that.

> 
> > The other issue is that if we are to apply more than 
> > one substitution on i2pat and i0dest_in_i0src, then we need to pass
> > 1 as last argument to the first subst in order to avoid unwanted
> > rtl sharing (which again can lead to self-referential rtl).
> > Another issue is that if all of i0_feeds_i2_n, i0_feeds_i1_n and
> > i1_feeds_i2_n is true, then we'd be substituting i0dest with i0src
> > in i2pat twice.
> 
> This part looks OK to me.
> 
> 
> 	* combine.c (try_combine): Fix formatting issues and a pasto.

Yeah, the

> -      newpat = subst (newpat, i0dest, i0src, 0,
> -                   i0_feeds_i1_n && i0dest_in_i0src);
> +      newpat = subst (newpat, i0dest, i0src, 0, 0);

is what I came across too and it surprised me, but I decided it
doesn't break anything, just is inefficient.  But you're right it is better
to fix it to make the code more readable.

	Jakub