[RFA,PR,middle-end/57904,P1,regression] Improve cleanups after copyprop

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

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
> +
>