This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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
exposes.

Many thanks in advance,

Roger
--


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]