cleanup cross product code in VRP

Jeff Law law@redhat.com
Thu Jul 19 14:45:00 GMT 2018


On 07/19/2018 03:06 AM, Aldy Hernandez wrote:
> 
> 
> On 07/19/2018 04:18 AM, Richard Biener wrote:
>> On Wed, Jul 18, 2018 at 2:05 PM Aldy Hernandez <aldyh@redhat.com> wrote:
>>>
>>> Hi again!
>>>
>>> Well, since this hasn't been reviewed and I'm about to overhaul the
>>> TYPE_OVERFLOW_WRAPS code anyhow, might as well lump it all in one patch.
>>>
>>> On 07/16/2018 09:19 AM, Aldy Hernandez wrote:
>>>> Howdy!
>>>>
>>>> I've abstracted out the cross product calculations into its own
>>>> function, and have adapted it to deal with wide ints so it's more
>>>> reusable.  It required some shuffling around, and implementing things a
>>>> bit different, but things should be behave as before.
>>>>
>>>> I also renamed vrp_int_const_binop to make its intent clearer,
>>>> especially now that it's really just a wrapper to wide_int_binop that
>>>> deals with overflow.
>>>>
>>>> (If wide_int_binop_overflow is generally useful, perhaps we could merge
>>>> it with wide_int_overflow.)
>>>
>>> This is the same as the previous patch, plus I'm abstracting the
>>> TYPE_OVERFLOW_WRAPS code as well.  With this, the code dealing with
>>> MULT_EXPR in vrp gets reduced to handling value_range specific stuff.
>>> Yay code re-use!
>>>
>>> A few notes:
>>>
>>> This is dead code.  I've removed it:
>>>
>>> -      /* If we have an unsigned MULT_EXPR with two VR_ANTI_RANGEs,
>>> -        drop to VR_VARYING.  It would take more effort to compute a
>>> -        precise range for such a case.  For example, if we have
>>> -        op0 == 65536 and op1 == 65536 with their ranges both being
>>> -        ~[0,0] on a 32-bit machine, we would have op0 * op1 == 0, so
>>> -        we cannot claim that the product is in ~[0,0].  Note that we
>>> -        are guaranteed to have vr0.type == vr1.type at this
>>> -        point.  */
>>> -      if (vr0.type == VR_ANTI_RANGE
>>> -         && !TYPE_OVERFLOW_UNDEFINED (expr_type))
>>> -       {
>>> -         set_value_range_to_varying (vr);
>>> -         return;
>>> -       }
>>>
>>> Also, the vrp_int typedef has a weird name, especially when we have
>>> widest2_int in gimple-fold.c that does the exact thing.  I've moved the
>>> common code to wide-int.h and tree.h so we can all share :).
>>>
>>> At some point we could move the wide_int_range* and wide_int_binop* code
>>> into its own file.
>>
>> Yes.
> 
> Sometime within the next couple rounds I'll come up with a file name
> that doesn't hurt my eyes.  It seems that the hardest part of
> programming is actually coming up with sensible file and variable names
> :-/.
I recall reading somewhere that once you have the right
function/method/variable names you're 90% of the way to having the
problem solved.  I don't totally agree, but I can see the point the
original author of the statement was trying to get across.

Internally when refactoring I usually start with calling stuff "foo",
"bar", "doit" and friends until I'm fairly happy with what got carved
out then I try to figure out reasonable names.

Jeff



More information about the Gcc-patches mailing list