[PATCH] Move beef from simplify_comparison to simplify_relational_operation

[Roger Sayle]

On Mon, 26 Apr 2004, Paolo Bonzini wrote:
> Finally, I removed a place where fold transformed unsigned comparisons
> to signed.  This is not appropriate in my opinion, because it causes the
> original semantics to be hidden at the tree level, which is especially
> important in tree-ssa (I implemented constant folding to create
> saturated minus and plus tree nodes, and this particular transform got
> in the way in the famous gzip case).

I disagree with you on this particular hunk.  Its not unreasonable for
the middle-end to canonicalize unsigned comparisons against INT_MAX into
signed comparisons against zero.  The semantics aren't in anyway hidden
and two different operations that have identical behaviour are now
represented the same "tree".  Not to mention the performance benefits
of prefering comparisons against zero.  As you mention above, this
constant folding transformation greatly helps SPECint2000's 164.gzip.

I suspect that the problem is in your implementation of saturated addition
and subtraction, where your tests need to be tweaked to realize that
signed comparison against zero really is equivalent to unsigned
comparison against INT_MAX.  fold's canonicalization should actually
help here as you won't need to also explicitly test for "user-coded"
signed comparisons against zero.

At the very least, this hunk should have been submitted separately
and not buried within a major reorganization.  It would also need
to be supported by benchmarking figures to show the impact of removing
this optimization.  I'm guessing that without your saturated operation
patch, the results wouldn't look too good.  However, as I point out
above, I really believe that the problem is elsewhere.


In addition to the overly long lines, there are one or two errors
in your patch.


> Index: integrate.c
> ===================================================================
> RCS file: /cvs/gcc/gcc/gcc/integrate.c,v
> retrieving revision 1.254
> diff -u -r1.254 integrate.c
> --- integrate.c 1 Apr 2004 03:50:29 -0000       1.254
> +++ integrate.c 26 Apr 2004 14:17:58 -0000
> @@ -2738,8 +2738,10 @@
>
>          if (op_mode == VOIDmode)
>            op_mode = GET_MODE (XEXP (x, 1));
> -         new = simplify_relational_operation (code, GET_MODE (x), op_mode,
> -                                              XEXP (x, 0), XEXP (x, 1));
> +
> +         new = simplify_relational_operation (code, op_mode, GET_MODE (x),
> +                                              XEXP (x, 0), XEXP (x, 1),
> +                                              NULL);
>           break;
>         }


Here, for example, you've swapped the GET_MODE (x) and the op_mode
arguments.  The first needs to be the mode of the result GET_MODE (x),
and the second needs to be the mode of the comparison op_mode.

>From the ChangeLog, the entry for integrate.c reads:

>	* integrate.c (subst_constants): simplify_relational_operation
>	now takes care of FLOAT_STORE_FLAG_VALUE.

But you'll notice from the diff, subst_constants didn't worry about
FLOAT_STORE_FLAG_VALUE before or after this change.

Also in the integrate.c changes

> +               rtx tem =
> +                 simplify_relational_operation (GET_CODE (op0),
> +                                                GET_MODE (op0),
> +                                                map->compare_mode,
> +                                                XEXP (op0, 0),
> +                                                XEXP (op0, 1),
> +                                                &result);
>
> -               if (temp == const0_rtx)
> +               if (result == 0)
>                   new = XEXP (x, 2);
> -               else if (temp == const1_rtx)
> +               else if (result == 1)
>                   new = XEXP (x, 1);
> +               else
> +                 new = simplify_ternary_operation (code, GET_MODE (x), op0_mode,
> +                                                   tem, XEXP (x, 1), XEXP (x, 2));

The implementation of simplify_relational_operation can potentially
return a NULL_RTX.  Notice in the original code, if the comparison wasn't
fully simplified to either true or false, it did nothing.  After your,
patch we can return a NULL_RTX (with result set to -1) and we pass "tem"
to simplify_ternary_operation without ever checking whether tem is NULL.

Likewise in this code:

> -         /* See if any simplifications were possible.  */
> -         if (temp == const0_rtx)
> -           return op2;
> -         else if (temp == const_true_rtx)
> -           return op1;
> -         else if (temp)
> -           abort ();

Here you're removing an assertion that the condition must be simplified
to either true or false.  And instead replacing it with..

> +         /* See if any simplifications were possible.  */
> +         if (result == 0)
> +           return op2;
> +         else if (result == 1)
> +           return op1;
> +         else if (temp)
> +           return gen_rtx_IF_THEN_ELSE (mode, temp, op1, op2);


This is clearly a functional change.  We no longer test our the
validity of our invariant, and it isn't immediately clear that the
caller could handle an IF_THEN_ELSE rtx as a result.  Even if it
could, the hunk reviewed above shows that the correct way to generate
an IF_THEN_ELSE rtx here is via simplify_ternary_operation.


There are however plenty of good clean-ups in your patch, and the
primary goal of moving conditional simplification into the body
of simplify_relational_operation is indeed the right thing to do.


Are you sure the simplify_relational_operation function really
needs the additional "int *result" argument?  One might argue that
everywhere that "result" is significant, we should probably have
been calling relational_operation_const_result.  Otherwise it's
simple enough to check whether GET_CODE of the result is CONST_INT.


There's also the question of what do we gain by changing the API
of simplify_const_relational_operation from returning either const0_rtx,
const_true_rtx or NULL_RTX, into returning either 0, 1 or -1 under
equivalent circumstances, respectively.  Indeed, as you point out,
changing the return type to int means that it no longer behaves like a
simplify_* function, so it requires renaming.  In my view, the meaning
of a -1 return value of relational_operation_const_result is no clearer
than the well known interpretation of returning NULL_RTX from a simplify_
function.


I think this patch is trying to attempt three (or four) independent
things, and suffering from attempting them all at once.

[1] The transition of transformations into simplify-rtx.c.
[2] Modifying existing callers of simplify_const_relational_operation
    to instead call simplify_relational_operation, e.g. the dojump.c
    changes.
[3] Change the calling conventions of these two functions, primarily
    to simplify step [2].

Each of these three changes could probably be submitted as independent
patches.  This makes them easier to review, and should the worst come
to the worst, easier to revert should any introduce problems.


Please feel free to comment on any of the points above.  Constructively,
however, could you prepare a patch to "move the beef" without changing
the APIs/parameters to either simplify_const_relational_operation or
simplify_relational_operation and without removing the signed->unsigned
comparison optimization from fold?  This may require some functional
changes in some callers, but anything that doesn't need to change should
remain unchanged to minimize problems.

I hope this makes sense.  Let me know what you think.
Thanks for attempting this clean-up, it really needs to get done.

Roger
--