This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: abstract wide int binop code from VRP


Aldy Hernandez <aldyh@redhat.com> writes:
> On 07/11/2018 01:33 PM, Richard Sandiford wrote:
>> 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;
>  >
>  > 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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]