This is the mail archive of the
mailing list for the GCC project.
Re: [PATCH] Fix combiner (PRs rtl-optimization/46034, rtl-optimization/46212, rtl-optimization/46248)
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.
> - 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.