From: Richard Biener <richard.guenther@gmail.com>
To: Jeff Law <law@redhat.com>
Cc: gcc-patches <gcc-patches@gcc.gnu.org>
Subject: Re: [RFA][PATCH][PR middle-end/57904][P1 regression] Improve cleanups after copyprop
Date: Fri, 17 Jan 2014 10:45:00 -0000 [thread overview]
Message-ID: <CAFiYyc2FtGG7-KGnXUOwmJqqKqgBhU3ZeHV_W=fax4UDHyr9dQ@mail.gmail.com> (raw)
In-Reply-To: <52D82533.2030300@redhat.com>
On Thu, Jan 16, 2014 at 7:30 PM, Jeff Law <law@redhat.com> wrote:
> On 01/16/14 04:49, Richard Biener wrote:
>>
>>
>> Well - the issue here is that inlining / IPA-CP propagates constant
>> arguments to direct uses which of course exposes constant propagation
>> opportunities. Now, copyprop doesn't to "real" constant propagation,
>> it just also propagates constants as if they were registers.
>>
>> So it exactly works as designed, but you could argue that pass
>> ordering
>>
>> NEXT_PASS (pass_copy_prop);
>> NEXT_PASS (pass_complete_unrolli);
>> NEXT_PASS (pass_ccp);
>>
>> is wrong. Of course complete unrolling exposes constant propagation
>> opportunities (though nowadays it has a cheap CCP machinery built-in).
>>
>> IIRC that copyprop pass was added to avoid spurious warnings just
>> as in the PR. You could argue that with complete unrolling having
>> a cheap CCP built in (see propagate_constants_for_unrolling) we
>> should move CCP before unrolli (and copyprop!) as well.
>
> It's certainly possible that copyprop was added for that reason, I simply
> have no memory of it.
>
> I tend to be leery of juggling passes simply because it's often just pushing
> the bubble down in one spot and making another appear elsewhere. However, I
> don't feel that strongly about it in this case.
>
>
>>
>> So - please try making pass order
>>
>> NEXT_PASS (pass_ccp);
>> NEXT_PASS (pass_copy_prop);
>> NEXT_PASS (pass_complete_unrolli);
>>
>> instead.
>
> That fixes things as well. Bootstrapped and regression tested. OK for the
> trunk?
Ok.
Thanks,
Richard.
>
>
> commit 4e47e40685c4480945783e77ebc9a123d15cfd24
> Author: Jeff Law <law@redhat.com>
> Date: Thu Jan 16 11:20:42 2014 -0700
>
> PR middle-end/57904
> * passes.def: Reorder pass_copy_prop, pass_unrolli, pass_ccp
> sequence
> so that pass_ccp runs first.
>
> PR middle-end/57904
> * gfortran.dg/pr57904.f90: New test.
>
> diff --git a/gcc/ChangeLog b/gcc/ChangeLog
> index d4f83f4..6669f26 100644
> --- a/gcc/ChangeLog
> +++ b/gcc/ChangeLog
> @@ -1,3 +1,9 @@
> +2014-01-16 Jeff Law <law@redhat.com>
> +
> + PR middle-end/57904
> + * passes.def: Reorder pass_copy_prop, pass_unrolli, pass_ccp
> sequence
> + so that pass_ccp runs first.
> +
> 2014-01-16 Alan Lawrence <alan.lawrence@arm.com>
>
> * config/arm/arm.opt: Make -mcpu, -march, -mtune case-insensitive.
> diff --git a/gcc/passes.def b/gcc/passes.def
> index 95ea8ce..c98b048 100644
> --- a/gcc/passes.def
> +++ b/gcc/passes.def
> @@ -132,11 +132,11 @@ along with GCC; see the file COPYING3. If not see
> They ensure memory accesses are not indirect wherever possible. */
> NEXT_PASS (pass_strip_predict_hints);
> NEXT_PASS (pass_rename_ssa_copies);
> - NEXT_PASS (pass_copy_prop);
> - NEXT_PASS (pass_complete_unrolli);
> NEXT_PASS (pass_ccp);
> /* After CCP we rewrite no longer addressed locals into SSA
> form if possible. */
> + NEXT_PASS (pass_copy_prop);
> + NEXT_PASS (pass_complete_unrolli);
> NEXT_PASS (pass_phiprop);
> NEXT_PASS (pass_forwprop);
> NEXT_PASS (pass_object_sizes);
> diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog
> index 868593b..65a37b5 100644
> --- a/gcc/testsuite/ChangeLog
> +++ b/gcc/testsuite/ChangeLog
> @@ -1,3 +1,8 @@
> +2014-01-16 Jeff Law <law@redhat.com>
> +
> + PR middle-end/57904
> + * gfortran.dg/pr57904.f90: New test.
> +
> 2014-01-16 Nick Clifton <nickc@redhat.com>
>
> PR middle-end/28865
> diff --git a/gcc/testsuite/gfortran.dg/pr57904.f90
> b/gcc/testsuite/gfortran.dg/pr57904.f90
> new file mode 100644
> index 0000000..69fa7ed
> --- /dev/null
> +++ b/gcc/testsuite/gfortran.dg/pr57904.f90
> @@ -0,0 +1,22 @@
> +! { dg-do compile }
> +! { dg-options "-O2" }
> +
> +program test
> + call test2 ()
> +contains
> + subroutine test2 ()
> + type t
> + integer, allocatable :: x
> + end type t
> +
> + type t2
> + class(t), allocatable :: a
> + end type t2
> +
> + type(t2) :: one, two
> +
> + allocate (two%a)
> + one = two
> + end subroutine test2
> +end program test
> +
>
prev parent reply other threads:[~2014-01-17 10:45 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-01-15 21:39 Jeff Law
2014-01-15 22:16 ` Marek Polacek
2014-01-16 5:50 ` Jeff Law
2014-01-16 11:49 ` Richard Biener
2014-01-16 18:40 ` Jeff Law
2014-01-17 10:45 ` Richard Biener [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAFiYyc2FtGG7-KGnXUOwmJqqKqgBhU3ZeHV_W=fax4UDHyr9dQ@mail.gmail.com' \
--to=richard.guenther@gmail.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=law@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).