This is the mail archive of the
mailing list for the GCC project.
Re: Combine performance regression (was: Fix PR target/18701)
- From: Roger Sayle <roger at eyesopen dot com>
- To: Hans-Peter Nilsson <hp at bitrange dot com>
- Cc: Ulrich Weigand <uweigand at de dot ibm dot com>, Jakub Jelinek <jakub at redhat dot com>, <gcc-patches at gcc dot gnu dot org>
- Date: Tue, 25 Jan 2005 16:12:38 -0700 (MST)
- Subject: Re: Combine performance regression (was: Fix PR target/18701)
On Tue, 25 Jan 2005, Hans-Peter Nilsson wrote:
> On Thu, 20 Jan 2005, Roger Sayle wrote:
> > I think this entire clause in combine_simplify_rtx isn't required
> > and can be deleted. I'm currently bootstrapping and regression
> > testing that change.
> I assume you're talking about combine.c:1.471:3986:
> /* Don't change the mode of the MEM if that would change the meaning
> of the address. */
> if (MEM_P (SUBREG_REG (x))
> && (MEM_VOLATILE_P (SUBREG_REG (x))
> || mode_dependent_address_p (XEXP (SUBREG_REG (x), 0))))
> return gen_rtx_CLOBBER (mode, const0_rtx);
> (Which is either correct or likely what together with the other
> bug confused me into thinking that something else could
> somewhat-validly change the mode of the memory access.
> Why else would this code care whether the address is a
> mode_dependent_address_p? FWIW, no, I still haven't revisited
> this thoroughly yet.)
> How did that test go?
Completely removing the above clause from combine.c on mainline
bootstraps all default languages and regression tests with no new
failures on both i686-pc-linux-gnu and powerpc-unknown-linux-gnu.
I think this code, that was added by Jakub Jelinek to resolve
PR optimization/6086, was really papering over a bug elsewhere
in the compiler, more specifically in reload. Given that the
testcase that was added for that PR, g++.dg/opt/preinc1.C, now
still passes on PPC even with the above clause removed, I'd like
to believe that the real underlying problem has now been corrected.
Jakub, I was wondering whether you agree with the above thread?
I think this workaround can be now be removed for combine.c, lest
it cause the same sort of confusion as H-P's in future. If so,
I wonder if I could ask a favour, please could you to test deleting
the above clause (i.e. reverting the 1.278 to 1.279 change to
combine.c) and bootstrap/regression test on your "build farm"?
This kills two birds with one stone; (i) I wouldn't want to revert
Jakub's patch without being sure he agrees to it and (ii) I'd be
much happier if this kind of cosmetic missed-optimization clean-up
was fully tested on a range of platforms whilst in stage3. If we
can get mainline to regress with this change, I'll be happy to
investigate a true fix to the latent RTL optimizer problems it
Many thanks in advance,