Aldy Hernandez <al...@redhat.com> writes:
> On 07/11/2018 08:52 AM, Richard Biener wrote:
>> On Wed, Jul 11, 2018 at 8:48 AM Aldy Hernandez <al...@redhat.com> wrote:
>>>
>>> Hmmm, I think we can do better, and since this hasn't been reviewed yet,
>>> I don't think anyone will mind the adjustment to the patch ;-).
>>>
>>> I really hate int_const_binop_SOME_RANDOM_NUMBER. We should abstract
>>> them into properly named poly_int_binop, wide_int_binop, and tree_binop,
>>> and then use a default argument for int_const_binop() to get things going.
>>>
>>> Sorry for more changes in flight, but I thought we could benefit from
>>> more cleanups :).
>>>
>>> OK for trunk pending tests?
>>
>> Much of GCC pre-dates function overloading / default args ;)
>
> Heh...and ANSI C.
>
>>
>> Looks OK but can you please rename your tree_binop to int_cst_binop?
>> Or maybe inline it into int_const_binop, also sharing the force_fit_type ()
>> tail with poly_int_binop?
>
> I tried both, but inlining looked cleaner :). Done.
>
>>
>> What about mixed INTEGER_CST / poly_int constants? Shouldn't it
>> be
>>
>> if (neither-poly-nor-integer-cst (arg1 || arg2))
>> return NULL_TREE;
>> if (poly_int_tree (arg1) || poly_int_tree (arg2))
>> poly-int-stuff
>> else if (INTEGER_CST && INTEGER_CST)
>> wide-int-stuff
>>
>> ? I see that is a pre-existing issue but if you are at refactoring...
>> wi::to_poly_wide should handle INTEGER_CST operands just fine
>> I hope.
>
> This aborted:
> gcc_assert (NUM_POLY_INT_COEFFS != 1);
>
> but even taking it out made the bootstrap die somewhere else.
>
> If it's ok, I'd rather not tackle this now, as I have some more cleanups
> that are pending on this. If you feel strongly, I could do it at a
> later time.
>
> OK pending tests?
LGTM FWIW, just some nits:
> -/* Subroutine of int_const_binop_1 that handles two INTEGER_CSTs. */
> +/* Combine two wide ints ARG1 and ARG2 under operation CODE to produce
> + a new constant in RES. Return FALSE if we don't know how to
> + evaluate CODE at compile-time. */
>
> -static tree
> -int_const_binop_2 (enum tree_code code, const_tree parg1, const_tree parg2,
> - int overflowable)
> +bool
> +wide_int_binop (enum tree_code code,
> + wide_int &res, const wide_int &arg1, const wide_int &arg2,
> + signop sign, wi::overflow_type &overflow)
> {
IMO we should avoid pass-back by reference like the plague. :-)
It's especially confusing when the code does things like:
case FLOOR_DIV_EXPR:
if (arg2 == 0)
return false;
res = wi::div_floor (arg1, arg2, sign, &overflow);
break;
It looked at first like it was taking the address of a local variable
and failing to propagate the information back up.
I think we should stick to using pointers for this kind of thing.
> -/* Combine two integer constants PARG1 and PARG2 under operation CODE
> - to produce a new constant. Return NULL_TREE if we don't know how
> +/* Combine two poly int's ARG1 and ARG2 under operation CODE to
> + produce a new constant in RES. Return FALSE if we don't know how
> to evaluate CODE at compile-time. */
>
> -static tree
> -int_const_binop_1 (enum tree_code code, const_tree arg1, const_tree arg2,
> - int overflowable)
> +static bool
> +poly_int_binop (poly_wide_int &res, enum tree_code code,
> + const_tree arg1, const_tree arg2,
> + signop sign, wi::overflow_type &overflow)
> {
Would be good to be consistent about the order of the result and code
arguments. Here it's "result, code" (which seems better IMO),
but in wide_int_binop it's "code, result".
> +/* Combine two integer constants PARG1 and PARG2 under operation CODE
> + to produce a new constant. Return NULL_TREE if we don't know how
> + to evaluate CODE at compile-time. */
> +
> tree
> -int_const_binop (enum tree_code code, const_tree arg1, const_tree arg2)
> +int_const_binop (enum tree_code code, const_tree arg1, const_tree arg2,
> + int overflowable)
s/PARG/ARG/g in comment.
> {
> - return int_const_binop_1 (code, arg1, arg2, 1);
> + bool success = false;
> + poly_wide_int poly_res;
> + tree type = TREE_TYPE (arg1);
> + signop sign = TYPE_SIGN (type);
> + wi::overflow_type overflow = wi::OVF_NONE;
> +
> + if (TREE_CODE (arg1) == INTEGER_CST && TREE_CODE (arg2) == INTEGER_CST)
> + {
> + wide_int warg1 = wi::to_wide (arg1), res;
> + wide_int warg2 = wi::to_wide (arg2, TYPE_PRECISION (type));
> + success = wide_int_binop (code, res, warg1, warg2, sign, overflow);
> + poly_res = res;
> + }
> + else if (poly_int_tree_p (arg1) && poly_int_tree_p (arg2))
> + success = poly_int_binop (poly_res, code, arg1, arg2, sign, overflow);
> + if (success)
> + return force_fit_type (type, poly_res, overflowable,
> + (((sign == SIGNED || overflowable == -1)
> + && overflow)
> + | TREE_OVERFLOW (arg1) | TREE_OVERFLOW (arg2)));
> + return NULL_TREE;
> }
>
> /* Return true if binary operation OP distributes over addition in operand
> @@ -1925,7 +1930,7 @@ size_binop_loc (location_t loc, enum tree_code code,
> tree arg0, tree arg1)
> /* Handle general case of two integer constants. For sizetype
> constant calculations we always want to know about overflow,
> even in the unsigned case. */
> - tree res = int_const_binop_1 (code, arg0, arg1, -1);
> + tree res = int_const_binop (code, arg0, arg1, -1);
> if (res != NULL_TREE)
> return res;
> }
> diff --git a/gcc/fold-const.h b/gcc/fold-const.h
> index 4613a62e1f6..b921ba86854 100644
> --- a/gcc/fold-const.h
> +++ b/gcc/fold-const.h
> @@ -100,7 +100,10 @@ extern tree fold_bit_and_mask (tree, tree, enum
> tree_code,
> tree, enum tree_code, tree, tree,
> tree, enum tree_code, tree, tree, tree *);
> extern tree fold_read_from_constant_string (tree);
> -extern tree int_const_binop (enum tree_code, const_tree, const_tree);
> +extern bool wide_int_binop (enum tree_code, wide_int &res,
> + const wide_int &arg1, const wide_int &arg2,
> + signop, wi::overflow_type &);
> +extern tree int_const_binop (enum tree_code, const_tree, const_tree, int =
> 1);
> #define build_fold_addr_expr(T)\
> build_fold_addr_expr_loc (UNKNOWN_LOCATION, (T))
> extern tree build_fold_addr_expr_loc (location_t, tree);
> diff --git a/gcc/tree-vrp.c b/gcc/tree-vrp.c
> index a7453abba7f..7764f7b30b6 100644
> --- a/gcc/tree-vrp.c
> +++ b/gcc/tree-vrp.c
> @@ -956,11 +956,13 @@ value_range_constant_singleton (value_range *vr)
> return NULL_TREE;
> }
>
> -/* Wrapper around int_const_binop. Return true if we can compute the
> - result; i.e. if the operation doesn't overflow or if the overflow is
> - undefined. In the latter case (if the operation overflows and
> - overflow is undefined), then adjust the result to be -INF or +INF
> - depending on CODE, VAL1 and VAL2. Return the value in *RES.
> +/* Wrapper around wide_int_binop that adjusts for overflow.
> +
> + Return true if we can compute the result; i.e. if the operation
> + doesn't overflow or if the overflow is undefined. In the latter
> + case (if the operation overflows and overflow is undefined), then
> + adjust the result to be -INF or +INF depending on CODE, VAL1 and
> + VAL2. Return the value in *RES.
>
> Return false for division by zero, for which the result is
> indeterminate. */
> @@ -970,78 +972,36 @@ vrp_int_const_binop (enum tree_code code, tree val1,
> tree val2, wide_int *res)
> {
> wi::overflow_type overflow = wi::OVF_NONE;
> signop sign = TYPE_SIGN (TREE_TYPE (val1));
> + wide_int w1 = wi::to_wide (val1);
> + wide_int w2 = wi::to_wide (val2);
>
This doesn't come for free. Until we can use "auto", it might be better
to keep the wi::to_wide calls at the point they're used. That probably
maans having a separate call to wide_int_binop for shifts.
Thanks,
Richard
