Aldy Hernandez <aldyh@redhat.com> writes:
On 07/11/2018 08:52 AM, Richard Biener wrote:
On Wed, Jul 11, 2018 at 8:48 AM Aldy Hernandez <aldyh@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;