[Bug tree-optimization/62217] [4.9/5 Regression] DOM confuses complete unrolling which in turn causes VRP to warn

[rguenther at suse dot de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=62217

--- Comment #10 from rguenther at suse dot de <rguenther at suse dot de> ---
On Mon, 16 Feb 2015, law at redhat dot com wrote:

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=62217
> 
> --- Comment #9 from Jeffrey A. Law <law at redhat dot com> --- Yes, any 
> particular choice has the potential to regress in one way or another. 
> These are heuristics after all.  We're just looking for a reasonable 
> refinement if we can find one.

Well, there is also canonicalization for CSE / jump threading.  Consider

if (i == 2)
 {
   if (i != j)
     ...
   else if (j == 2)
     ...

or

y = 2 * i;
if (i == j)
  x = i + j;

but yes, this is followup transforms.  Unfortunately(?) DOM performs
copy/constant propagation for all recorded equalities.

> Dominance doesn't seem to be the right thing to be looking at to me 
> since the crux of this issue is propagating the "copy" implied by the 
> equality comparison just changes what SSA_NAME we reference and as a 
> result ultimately hides stuff from later passes.  It doesn't (in this 
> case) enable further simplifications or for either SSA_NAME to become 
> unused.  A dominance test between the args of the equality comparison 
> just doesn't seem helpful here.
>
> In fact, because both names are used in the equality test, these 
> propagations can never cause an SSA_NAME to become unused.  At best the 
> propagation will expose some further simplification on one of the paths 
> or it will result in one SSA_NAME having a single use (the comparison).  
> We have no good way of guessing if the former will happen, but we can 
> encourage the latter.

As you say, we don't know - we only know that properly canonicalizing
will expose the followup transforms if there are any.  It looks like
we are basically taking the original order of EQ_EXPR operands
(thus eventually rely on tree_swap_operands canonicalization) plus
the "correctness" thing of taking into account loop depth (which is
kind of a dominance relation).

We are also introducing SSA_NAME_VALUE "chains" in record_equality
as we are setting x SSA_NAME_VALUE to y not to SSA_NAME_VALUE (y)
(we seem to do this in multiple places for some odd reason...,
only tree-ssa-threadedge.c:record_temporary_equivalence seems to get
this right).

> But as I mentioned earlier, I really wonder if we should allow these context
> sensitive equivalences to be expressed in the gimple if they didn't prove
> useful.  And that was the whole purpose behind uncprop -- to find context
> sensitive propagations that ultimately didn't prove useful and which result in
> poor coalescing or unwanted constant initializations and un propagate them.

Yes (but on GIMPLE we don't care).

> It wouldn't directly help this problem because the use is in a normal
> statement, but it's definitely closely related and it wouldn't be hard to
> repurpose that code.  In fact, it might be a good thing to do in general.
> 
> Essentially what it's doing is building a map of values back to SSA_NAMEs which
> hold those values by way of an equality comparison.  At each use of the value,
> we can look at the list of SSA_NAMEs that hold that value and select the one
> that appears to be least cost based on whatever metrics we see fit.