Re: abstract wide int binop code from VRP
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?
Aldy
On 07/10/2018 04:31 AM, Aldy Hernandez wrote:
Howdy!
Attached are more cleanups to VRP getting rid of some repetitive code,
as well as abstracting wide int handling code into their own functions.
There should be no change to existing functionality.
You may notice that I have removed the PLUS/MINUS_EXPR handling in
vrp_int_const_binop, even from the new abstracted code:
- /* For addition, the operands must be of the same sign
- to yield an overflow. Its sign is therefore that
- of one of the operands, for example the first. */
- || (code == PLUS_EXPR && sgn1 >= 0)
- /* For subtraction, operands must be of
- different signs to yield an overflow. Its sign is
- therefore that of the first operand or the opposite of
- that of the second operand. A first operand of 0 counts
- as positive here, for the corner case 0 - (-INF), which
- overflows, but must yield +INF. */
- || (code == MINUS_EXPR && sgn1 >= 0)
This code is actually unreachable, as the switch above this snippet was
already aborting if code was not one of the shift or mult/div operators.
Oh yeah, don't blame me for the cryptic comment to
range_easy_mask_min_mask(). That machine language comment was already
there ;-).
OK pending one more round of tests?
Aldy
gcc/
* fold-const.c (int_const_binop_1): Abstract...
(wide_int_binop): ...wide int code here.
(poly_int_binop): ...poly int code here.
(tree_binop): ...tree code here.
* fold-const.h (wide_int_binop): New.
* tree-vrp.c (vrp_int_const_binop): Call wide_int_binop.
Remove useless PLUS/MINUS_EXPR case.
(zero_nonzero_bits_from_vr): Move wide int code...
(zero_nonzero_bits_from_bounds): ...here.
(extract_range_from_binary_expr_1): Move mask optimization code...
(range_easy_mask_min_max): ...here.
* tree-vrp.h (zero_nonzero_bits_from_bounds): New.
(range_easy_mask_min_max): New.
diff --git a/gcc/fold-const.c b/gcc/fold-const.c
index 97c435fa5e0..4614b8adcf4 100644
--- a/gcc/fold-const.c
+++ b/gcc/fold-const.c
@@ -966,21 +966,17 @@ int_binop_types_match_p (enum tree_code code, const_tree type1, const_tree type2
&& TYPE_MODE (type1) == TYPE_MODE (type2);
}
-/* 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)
{
- wide_int res;
- tree t;
- tree type = TREE_TYPE (parg1);
- signop sign = TYPE_SIGN (type);
- wi::overflow_type overflow = wi::OVF_NONE;
-
- wi::tree_to_wide_ref arg1 = wi::to_wide (parg1);
- wide_int arg2 = wi::to_wide (parg2, TYPE_PRECISION (type));
-
+ wide_int tmp;
+ overflow = wi::OVF_NONE;
switch (code)
{
case BIT_IOR_EXPR:
@@ -999,37 +995,41 @@ int_const_binop_2 (enum tree_code code, const_tree parg1, const_tree parg2,
case LSHIFT_EXPR:
if (wi::neg_p (arg2))
{
- arg2 = -arg2;
+ tmp = -arg2;
if (code == RSHIFT_EXPR)
code = LSHIFT_EXPR;
else
code = RSHIFT_EXPR;
}
+ else
+tmp = arg2;
if (code == RSHIFT_EXPR)
/* It's unclear from the C standard whether shifts can overflow.
The following code ignores overflow; perhaps a C standard
interpretation ruling is needed. */
- res = wi::rshift (arg1, arg2, sign);
+ res = wi::rshift (arg1, tmp, sign);
else
- res = wi::lshift (arg1, arg2);
+ res = wi::lshift (arg1, tmp);
break;
case RROTATE_EXPR:
case LROTATE_EXPR:
if (wi::neg_p (arg2))
{
- arg2 = -arg2;
+ tmp = -arg2;
if (code == RROTATE_EXPR)
code = LROTATE_EXPR;
else
code = RROTATE_EXPR;
}
+ else
+tmp = arg2;
if (code == RROTATE_EXPR)
- res = wi::rrotate (arg1, arg2);
+ res = wi::rrotate (arg1, tmp);
else
- res = wi::lrotate (arg1, arg2);
+ res = wi::lrotate (arg1, tmp);
break;
case PLUS_EXPR:
@@ -1051,49 +1051,49 @@ int_const_binop_2 (enum tree_code code, const_tree parg1, const_tree parg2,
case TRUNC_DIV_EXPR:
case EXACT_DIV_EXPR:
if (arg2 == 0)
- return NULL_TREE;
+ re
Re: abstract wide int binop code from VRP
On Wed, Jul 11, 2018 at 8:48 AM Aldy Hernandez 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 ;) 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? 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. Thanks, Richard. > Aldy > > On 07/10/2018 04:31 AM, Aldy Hernandez wrote: > > Howdy! > > > > Attached are more cleanups to VRP getting rid of some repetitive code, > > as well as abstracting wide int handling code into their own functions. > > There should be no change to existing functionality. > > > > You may notice that I have removed the PLUS/MINUS_EXPR handling in > > vrp_int_const_binop, even from the new abstracted code: > > > > - /* For addition, the operands must be of the same sign > > - to yield an overflow. Its sign is therefore that > > - of one of the operands, for example the first. */ > > - || (code == PLUS_EXPR && sgn1 >= 0) > > - /* For subtraction, operands must be of > > - different signs to yield an overflow. Its sign is > > - therefore that of the first operand or the opposite of > > - that of the second operand. A first operand of 0 counts > > - as positive here, for the corner case 0 - (-INF), which > > - overflows, but must yield +INF. */ > > - || (code == MINUS_EXPR && sgn1 >= 0) > > > > This code is actually unreachable, as the switch above this snippet was > > already aborting if code was not one of the shift or mult/div operators. > > > > Oh yeah, don't blame me for the cryptic comment to > > range_easy_mask_min_mask(). That machine language comment was already > > there ;-). > > > > OK pending one more round of tests? > > > > Aldy
Re: abstract wide int binop code from VRP
On 07/11/2018 08:52 AM, Richard Biener wrote:
On Wed, Jul 11, 2018 at 8:48 AM Aldy Hernandez 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?
Aldy
gcc/
* fold-const.c (int_const_binop_1): Abstract...
(wide_int_binop): ...wide int code here.
(poly_int_binop): ...poly int code here.
Abstract the rest of int_const_binop_1 into int_const_binop.
* fold-const.h (wide_int_binop): New.
* tree-vrp.c (vrp_int_const_binop): Call wide_int_binop.
Remove useless PLUS/MINUS_EXPR case.
(zero_nonzero_bits_from_vr): Move wide int code...
(zero_nonzero_bits_from_bounds): ...here.
(extract_range_from_binary_expr_1): Move mask optimization code...
(range_easy_mask_min_max): ...here.
* tree-vrp.h (zero_nonzero_bits_from_bounds): New.
(range_easy_mask_min_max): New.
diff --git a/gcc/fold-const.c b/gcc/fold-const.c
index 97c435fa5e0..ad8c0a69f63 100644
--- a/gcc/fold-const.c
+++ b/gcc/fold-const.c
@@ -966,21 +966,17 @@ int_binop_types_match_p (enum tree_code code, const_tree type1, const_tree type2
&& TYPE_MODE (type1) == TYPE_MODE (type2);
}
-/* 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)
{
- wide_int res;
- tree t;
- tree type = TREE_TYPE (parg1);
- signop sign = TYPE_SIGN (type);
- wi::overflow_type overflow = wi::OVF_NONE;
-
- wi::tree_to_wide_ref arg1 = wi::to_wide (parg1);
- wide_int arg2 = wi::to_wide (parg2, TYPE_PRECISION (type));
-
+ wide_int tmp;
+ overflow = wi::OVF_NONE;
switch (code)
{
case BIT_IOR_EXPR:
@@ -999,37 +995,41 @@ int_const_binop_2 (enum tree_code code, const_tree parg1, const_tree parg2,
case LSHIFT_EXPR:
if (wi::neg_p (arg2))
{
- arg2 = -arg2;
+ tmp = -arg2;
if (code == RSHIFT_EXPR)
code = LSHIFT_EXPR;
else
code = RSHIFT_EXPR;
}
+ else
+tmp = arg2;
if (code == RSHIFT_EXPR)
/* It's unclear from the C standard whether shifts can overflow.
The following code ignores overflow; perhaps a C standard
interpretation ruling is needed. */
- res = wi::rshift (arg1, arg2, sign);
+ res = wi::rshift (arg1, tmp, sign);
else
- res = wi::lshift (arg1, arg2);
+ res = wi::lshift (arg1, tmp);
break;
case RROTATE_EXPR:
case LROTATE_EXPR:
if (wi::neg_p (arg2))
{
- arg2 = -arg2;
+ tmp = -arg2;
if (code == RROTATE_EXPR)
code = LROTATE_EXPR;
else
code = RROTATE_EXPR;
}
+ else
+tmp = arg2;
if (code == RROTATE_EXPR)
- res = wi::rrotate (arg1, arg2);
+ res = wi::rrotate (arg1, tmp);
else
- res = wi::lrotate (arg1, arg2);
+ res = wi::lrotate (arg1, tmp);
break;
case PLUS_EXPR:
@@ -1051,49 +1051,49 @@ int_const_binop_2 (enum tree_code code, const_tree parg1, const_tree parg2,
case TRUNC_DIV_EXPR:
case EXACT_DIV_EXPR:
if (arg2 == 0)
- return NULL_TREE;
+ return false;
res = wi::div_trunc (arg1, arg2, sign, &overflow);
break;
case FLOOR_DIV_EXPR:
if (arg2 == 0)
- return NULL_TREE;
+ return false;
res
Re: abstract wide int binop code from VRP
Richard Biener writes:
> On Wed, Jul 11, 2018 at 8:48 AM Aldy Hernandez 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 ;)
>
> 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?
>
> 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.
Don't think it's a preexisting issue. poly_int_tree_p returns true
for anything that can be represented as a poly_int, i.e. both
INTEGER_CST and POLY_INT_CST. (It wouldn't really make sense to
ask whether something could *only* be represented as a POLY_INT_CST.)
So:
if (poly_int_tree_p (arg1) && poly_int_tree_p (arg2))
{
poly_wide_int res;
bool overflow;
tree type = TREE_TYPE (arg1);
signop sign = TYPE_SIGN (type);
switch (code)
{
case PLUS_EXPR:
res = wi::add (wi::to_poly_wide (arg1),
wi::to_poly_wide (arg2), sign, &overflow);
break;
handles POLY_INT_CST + POLY_INT_CST, POLY_INT_CST + INTEGER_CST and
INTEGER_CST + POLY_INT_CST.
Thanks,
Richard
Re: abstract wide int binop code from VRP
Aldy Hernandez writes:
> On 07/11/2018 08:52 AM, Richard Biener wrote:
>> On Wed, Jul 11, 2018 at 8:48 AM Aldy Hernandez 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_OVERFL
Re: abstract wide int binop code from VRP
On 07/11/2018 01:33 PM, Richard Sandiford wrote:
Aldy Hernandez writes:
On 07/11/2018 08:52 AM, Richard Biener wrote:
On Wed, Jul 11, 2018 at 8:48 AM Aldy Hernandez 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.
>
Hmmm, I kinda like them. It just takes some getting used to, but
generally yields cleaner code as you don't have to keep using '*'
everywhere. Plus, the callee can assume the pointer is non-zero.
-/* 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".
Agreed. Fixed.
+/* 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.
Fixed.
{
- 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)
+
Re: abstract wide int binop code from VRP
Aldy Hernandez writes:
> On 07/11/2018 01:33 PM, Richard Sandiford wrote:
>> Aldy Hernandez writes:
>>> On 07/11/2018 08:52 AM, Richard Biener wrote:
On Wed, Jul 11, 2018 at 8:48 AM Aldy Hernandez 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.
> >
>
> Hmmm, I kinda like them. It just takes some getting used to, but
> generally yields cleaner code as you don't have to keep using '*'
> everywhere. Plus, the callee can assume the pointer is non-zero.
But it can assume that for "*" too.
The problem isn't getting used to them. I've worked on codebases where
this is the norm before and had to live with it. It's just always felt
a mistake even then.
E.g. compare:
int_const_binop_1 (code, arg1, arg2, overflowable);
and:
wide_int_binop (code, res, arg1, arg2, sign, overflow);
There's just no visual clue to tell you that "overflowable" is an
input and "overflow" is an output. ("overflowable" could well be
an output from the raw meaning: "the calculation might have induced
an overflow, but we're not sure".)
I wouldn't mind so much if we had a convention that the outputs
had a suffix to make it clear that they were outputs. But that
would be more typing than "*".
Thanks,
Richard
Re: abstract wide int binop code from VRP
On 07/12/2018 04:29 AM, Richard Sandiford wrote:
Aldy Hernandez writes:
On 07/11/2018 01:33 PM, Richard Sandiford wrote:
Aldy Hernandez writes:
On 07/11/2018 08:52 AM, Richard Biener wrote:
On Wed, Jul 11, 2018 at 8:48 AM Aldy Hernandez 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.
>
Hmmm, I kinda like them. It just takes some getting used to, but
generally yields cleaner code as you don't have to keep using '*'
everywhere. Plus, the callee can assume the pointer is non-zero.
But it can assume that for "*" too.
The problem isn't getting used to them. I've worked on codebases where
this is the norm before and had to live with it. It's just always felt
a mistake even then.
E.g. compare:
int_const_binop_1 (code, arg1, arg2, overflowable);
and:
wide_int_binop (code, res, arg1, arg2, sign, overflow);
There's just no visual clue to tell you that "overflowable" is an
input and "overflow" is an output. ("overflowable" could well be
an output from the raw meaning: "the calculation might have induced
an overflow, but we're not sure".)
I wouldn't mind so much if we had a convention that the outputs
had a suffix to make it clear that they were outputs. But that
would be more typing than "*".
Perhaps a proposal for the coding conventions document? ;-)
Aldy
Re: abstract wide int binop code from VRP
On Thu, Jul 12, 2018 at 10:12 AM Aldy Hernandez wrote:
>
> On 07/11/2018 01:33 PM, Richard Sandiford wrote:
> > Aldy Hernandez writes:
> >> On 07/11/2018 08:52 AM, Richard Biener wrote:
> >>> On Wed, Jul 11, 2018 at 8:48 AM Aldy Hernandez 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.
> >
>
> Hmmm, I kinda like them. It just takes some getting used to, but
> generally yields cleaner code as you don't have to keep using '*'
> everywhere. Plus, the callee can assume the pointer is non-zero.
>
> >> -/* 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".
>
> Agreed. Fixed.
>
> >
> >> +/* 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.
>
> Fixed.
>
> >
> >> {
> >> - 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::ov
Re: abstract wide int binop code from VRP
On 07/13/2018 03:02 AM, Richard Biener wrote: On Thu, Jul 12, 2018 at 10:12 AM Aldy Hernandez wrote: So besides the general discussion about references/pointers for out parameters let's stay consistet within related APIs. This means wide_int_binop should have a wide_int wide_int_binop (enum tree_code, const wide_int &, const wide_int &, signop, wi::overflow_type *) signature. Notice how I elided the out wide_int parameter to be the return value which means the function isn't supposed to fail which means gcc_unreachable () for "unhandled" tree codes. wide_int_binop was returning failure for: case CEIL_DIV_EXPR: if (arg2 == 0) return false; res = wi::div_ceil (arg1, arg2, sign, &overflow); break; case ROUND_DIV_EXPR: if (arg2 == 0) return false; res = wi::div_round (arg1, arg2, sign, &overflow); break; etc How do you suggest we indicate success/failure to the caller? Aldy It's more like an exceptional state anyway. The same goes for the poly_int_binop signature. The already existing wi::accumulate_overflow should probably be re-done as wi::overflow_type wi::accumulate_overflow (wi::overflow_type, wi::overflow_type); Richard. Thanks for the review! Aldy
Re: abstract wide int binop code from VRP
On Fri, Jul 13, 2018 at 10:05 AM Aldy Hernandez wrote: > > > > On 07/13/2018 03:02 AM, Richard Biener wrote: > > On Thu, Jul 12, 2018 at 10:12 AM Aldy Hernandez wrote: > > > So besides the general discussion about references/pointers for out > > parameters > > let's stay consistet within related APIs. This means wide_int_binop should > > have a > > > > wide_int > > wide_int_binop (enum tree_code, const wide_int &, const wide_int &, > > signop, wi::overflow_type *) > > > > signature. Notice how I elided the out wide_int parameter to be the > > return value which means > > the function isn't supposed to fail which means gcc_unreachable () for > > "unhandled" tree codes. > > wide_int_binop was returning failure for: > > > case CEIL_DIV_EXPR: > > if (arg2 == 0) > > return false; > > res = wi::div_ceil (arg1, arg2, sign, &overflow); > > break; > > > > case ROUND_DIV_EXPR: > > if (arg2 == 0) > > return false; > > res = wi::div_round (arg1, arg2, sign, &overflow); > > break; > etc > > How do you suggest we indicate success/failure to the caller? Oh, ok. Exceptions? (eh...) Well, so I guess you can leave the signature as-is apart from turing the overflow result into a pointer. OK with that change. Richard. > Aldy > > > It's more like an exceptional state anyway. > > > > The same goes for the poly_int_binop signature. > > > > The already existing wi::accumulate_overflow should probably be re-done as > > > > wi::overflow_type wi::accumulate_overflow (wi::overflow_type, > > wi::overflow_type); > > > > Richard. > > > >> Thanks for the review! > >> Aldy
Re: abstract wide int binop code from VRP
On Fri, Jul 13, 2018 at 3:18 PM Richard Biener wrote: > > On Fri, Jul 13, 2018 at 10:05 AM Aldy Hernandez wrote: > > > > > > > > On 07/13/2018 03:02 AM, Richard Biener wrote: > > > On Thu, Jul 12, 2018 at 10:12 AM Aldy Hernandez wrote: > > > > > So besides the general discussion about references/pointers for out > > > parameters > > > let's stay consistet within related APIs. This means wide_int_binop > > > should > > > have a > > > > > > wide_int > > > wide_int_binop (enum tree_code, const wide_int &, const wide_int &, > > > signop, wi::overflow_type *) > > > > > > signature. Notice how I elided the out wide_int parameter to be the > > > return value which means > > > the function isn't supposed to fail which means gcc_unreachable () for > > > "unhandled" tree codes. > > > > wide_int_binop was returning failure for: > > > > > case CEIL_DIV_EXPR: > > > if (arg2 == 0) > > > return false; > > > res = wi::div_ceil (arg1, arg2, sign, &overflow); > > > break; > > > > > > case ROUND_DIV_EXPR: > > > if (arg2 == 0) > > > return false; > > > res = wi::div_round (arg1, arg2, sign, &overflow); > > > break; > > etc > > > > How do you suggest we indicate success/failure to the caller? > > Oh, ok. Exceptions? (eh...) > > Well, so I guess you can leave the signature as-is apart from turing > the overflow > result into a pointer. Alternatively handle it like wi::sdiv and friends which indicate overflow and use a zero result. Thus, remove the "failure" path here. Of course when used via the tree interface this probably isn't the desired result which means this really isn't a general wide-int op with code thing but only a helper for int_cst_binop :/ So alternatively do the interface I suggested w/o the zero checks and do the zero checks in the int_const_binop caller. Richard. > OK with that change. > Richard. > > > Aldy > > > > > It's more like an exceptional state anyway. > > > > > > The same goes for the poly_int_binop signature. > > > > > > The already existing wi::accumulate_overflow should probably be re-done as > > > > > > wi::overflow_type wi::accumulate_overflow (wi::overflow_type, > > > wi::overflow_type); > > > > > > Richard. > > > > > >> Thanks for the review! > > >> Aldy
