Re: [RFA][PATCH][PR middle-end/57904][P1 regression] Improve cleanups after copyprop

[Richard Biener <richard dot guenther at gmail dot com>]

On Wed, Jan 15, 2014 at 10:39 PM, Jeff Law <law@redhat.com> wrote:
>
> Our SSA copy-prop passes do a pretty pathetic job at cleaning up after
> themselves when const/copy propagation exposes new trivial copies and
> constant initializations.
>
> This can be seen in the code for pr57904 after copyprop2 for bb2
>
>   _2 = 344;
>   ubound.0_3 = _2 & 7;
>   size.1_4 = ubound.0_3 + 1;
>   size.1_5 = MAX_EXPR <size.1_4, 0>;
>   _6 = size.1_5 * 4;
>   _7 = (character(kind=4)) _6;
>   _8 = MAX_EXPR <_7, 1>;
>   sizes_9 = __builtin_malloc (_8);
>   size.3_10 = MAX_EXPR <ubound.0_3, 0>;
>   _11 = size.3_10 * 4;
>   _12 = (character(kind=4)) _11;
>   _13 = MAX_EXPR <_12, 1>;
>   strides_14 = __builtin_malloc (_13);
>   MEM[(integer(kind=4)[0:D.1917] *)sizes_9][0] = 1;
>   if (ubound.0_3 > 0)
>     goto <bb 3>;
>   else
>     goto <bb 6>;
>
> We discover _2 = 344 and copyprop that to its uses.  However, copyprop does
> not discover that the RHS of ubound.0_3's assignment will simplify into a
> copy until *after* the main copy propagation algorithm is complete.  ie,
> that isn't discovered until subsitute_and_fold. Basically anything that
> doesn't look like a const/copy initialization when the pass starts is
> effectively ignored.

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.

I don't like how you make copyprop do more than what it is supposed
to do (in fact that I teached it to propagate constants at all also was
wrong ...).

In the end having separate copy and constant propagation passes
really is a red herring ... :/

So - please try making pass order

      NEXT_PASS (pass_ccp);
      NEXT_PASS (pass_copy_prop);
      NEXT_PASS (pass_complete_unrolli);

instead.

Thanks,
Richard.

> During substitute_and_fold, we substitute the value 344 for uses of _2.  But
> we don't pick up that ubound.0_3 now has a propagatable value. Worse yet,
> because we walk backwards through the statements, even if we recorded that
> ubound.0_3 has a propagatable value we wouldn't be able to utilize that
> information.
>
> For this particular PR, the lack of good optimization early results in an
> unreachable loop being left in the IL and that unreachable loop has
> undefined behaviour that we warn about.
>
>
> As it turns out the phi-only-cprop code can handle this quite easily.
>
> First we move all that stuff from tree-ssa-dom.c into tree-ssa-copy.c.
>
> Then we arrange (via callbacks) for tree-ssa-copy.c to get a notification
> when substitute_and_fold does something interesting.  In that callback we
> set an entry in a bitmap for any newly exposed copies or constant
> initializations.  That bitmap is then fed into the slightly refactored
> phi-only-cprop code as an initial state of potential copies/constant
> initializations.
>
> The net result is we collapse out all the necessarray code resulting in:
>
>
> <bb 2>:
> sizes_9 = __builtin_malloc (4);
> strides_14 = __builtin_malloc (1);
> MEM[(integer(kind=4)[0:D.1917] *)sizes_9][0] = 1;
> goto <bb 6>;
>
> bb3 (entry into the loop with undefined behaviour) is no longer reachable
> and the loop will get removed and we no longer issue the annoying warning.
>
>
> You'll note that I tightened the stmt_may_generate_copy code.  It was
> returning true for any binary RHS where the first operand was an invariant.
> Instead we return true for a UNARY/SINGLE rhs that is invariant (such as a
> casted ADDR_EXPR).
>
> Bootstrapped and regression tested on x86_64-unknown-linux-gnu.  OK for the
> trunk?
>