Re: [PATCH 2/3] Simplify wrapped binops

[Richard Biener <richard dot guenther at gmail dot com>]

On Wed, Jun 28, 2017 at 4:34 PM, Robin Dapp <rdapp@linux.vnet.ibm.com> wrote
>> ideally you'd use a wide-int here and defer the tree allocation to the result
>
> Did that in the attached version.
>
>> So I guess we never run into the outer_op == minus case as the above is
>> clearly wrong for that?
>
> Right, damn, not only was the treatment for this missing but it was
> bogus in the other pattern as well.  Since we are mostly dealing with
> PLUS_EXPR anyways it's probably better to defer the MINUS_EXPR case for
> now.  This will also slim down the patterns a bit.
>
>> try to keep vertical spacing in patterns minimal -- I belive that patterns
>> should be small enough to fit in a terminal window (24 lines).
>
> I find using the expanded wrapped_range condition in the simplification
> somewhat cumbersome, especially because I need the condition to evaluate
> to true by default making the initialization unintuitive.  Yet, I guess
> setting wrapped_range = true was not terribly intuitive either...

+     /* Perform binary operation inside the cast if the constant fits
+        and (A + CST)'s range does not wrap.  */
+     (with
+      {
+        bool min_ovf = true, max_ovf = false;

While the initialization value doesn't matter (wi::add will overwrite it)
better initialize both to false ;)  Ah, you mean because we want to
transform only if get_range_info returned VR_RANGE.  Indeed somewhat
unintuitive (but still the best variant for now).

+        wide_int w1 = @1;
+        w1 = w1.from (w1, TYPE_PRECISION (inner_type), TYPE_SIGN
+               (inner_type));

I think wi::from (@1, ....) should work as well.

+     (if (!((min_ovf && !max_ovf) || (!min_ovf && max_ovf)) )
+      (convert (plus @0 { {wide_int_to_tree (TREE_TYPE (@0), w1)}; })))

so I'm still missing a comment on why min_ovf && max_ovf is ok.
The simple-minded would have written

   (if  (! min_ovf && ! max_ovf)
...

I'd like to see testcase(s) with this patch, preferably exactly also for the
case of min_ovf && max_ovf.  That said, consider (long)[0xfffffffe,
0xffffffff] + 2
which should have min_ovf and max_ovf which results in [0x0, 0x1] in type
unsigned int but [0x100000000, 0x100000001] in type long.

Richard.

> Regards
>  Robin