Re: [patch] for PR 18529

[Roger Sayle <roger at eyesopen dot com>]

On Fri, 19 Nov 2004, Zdenek Dvorak wrote:
> no problem (although subst does not seem that terrible name for a
> function that performs substitution to me).

It's generally a good idea to avoid shadowing important function
names in GCC to avoid confusion, and simplify analysis of profiles.
For example, functions such as "build2", "fold", "try_combine",
"simplify_rtx", "cse_insn", "subst" etc... should probably never
be redeclared locally as static functions.

It's not that "subst" is a terrible name for a function, its just
that libbackend.a already contains a function called "subst" that
does something completely different.


> In this PR we deal with the expressions that are likely to never be
> emitted to the program.

I'll comment on your follow-up patch shortly.  But you're actually
missing optimization opportunities by not taking advantage of things
that you know about these trees that hold in tree-ssa-loop-niter.
A unique property of your usage is that you're asking fold to simplify
large (i.e. non-gimple) trees that contain ssa_names.  Normally, fold
has to deal with unbounded generic trees, or shallow gimple trees.
Hence the only time your transformation, as written could match is
from the calls from the loop optimizer.

As you've done in your later patch, this allows you to substitute
both ways without fear of exponential behaviour, and without adversely
affecting the performance of the rest of the compiler.

> My opinion is that if there is a simple generic solution, having
> special-purpose solutions to do it on each place where it is needed
> is a nice way to hell.

I disagree.  Your transformation as written can only trigger in calls
from tree-ssa-loop-niters, and is of no benefit and pure overhead to
every other caller of fold (of which there are several hundred).
If GCC internally requires a general "bool SAT" solver it probably
isn't appropriate to shoe-horn this into the already complex constant
folding machinery.  However, reusing fold's functionality as a subroutine
in such projects would be appropriate.

However it looks like your follow-up patch already addresses this issue
perfectly.


> > > +   if (!operand_equal_p (a1,
> > > + 			build2 (PLUS_EXPR, typea, a,
> > > + 				build_int_cst_type (typea, 1)), 0))
> > > +     return NULL_TREE;
> >
> >
> > This is much better written as
> >
> > 	if (TREE_CODE (a1) != PLUS_EXPR
> > 	    || !integer_onep (TREE_OPERAND (a1, 1))
> > 	    || !operand_equal_p (TREE_OPERAND (a1, 0), a, 0))
> > 	  return NULL_TREE.
> >
> > This avoids allocating tree nodes just to check that "a1 == a + 1".
>
> But is a bit weaker (does not handle the case when one of the
> expressions is x + 1 and the other one x + 2, for example).

Look more closely! You're not calling "fold" after you call "build2",
so the operand_equal_p can only return true for tree with a top-level
AND_EXPR, one of whose operands is integer_onep :>

If you did call "fold", I'd agree that as written your implementation
would be appropriate.  But if your testing and benchmarking has
already shown that patch as tested does what you want it to do, there's
a better way to test for your "weak" equivalence.


> > Could you explain why PR/18529 isn't better resolved elsewhere in
> > the compiler?
>
> Mostly since fold is the place where it is the most straightforward,
> and there also is a chance that other parts of the compiler will benefit
> from the transformations.

Not as written.  I was thinking in the case of PR/18529 that it might
be much simpler to rotate the loop at the same time as it is peeled.
I'm not an expert in such things, but it would appear to be simpler
to turn "for(a,b,c)" into "a;if(b){do c; while(b)}" in a single step,
instead of transforming it into "a;if(b)while(b)c;" and hoping that
later loop analysis will reveal the while iterates at least once?
There are probably good reasons, which is why I asked.

Roger
--