Re: [PATCH 2/3] Simplify wrapped binops

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

On Tue, Jun 20, 2017 at 3:08 PM, Robin Dapp <rdapp@linux.vnet.ibm.com> wrote:
>>> Currently, extract_... () does that all that for me, is it really too
>>> expensive to call? I guess, using get_range_info first and calling
>>> extract when get_range_info returns a VR_RANGE is not really a favorable
>>> thing to do either? :)
>> Not only the cost, we should avoid introducing more interfaces while
>> old ones can do the work.  Anyway, it's Richard's call here.
>
> I rewrote the match.pd patterns to use get_range_info () now, keeping
> track of an "ok" overflow (both min and max overflow) and one which does
> not allow us to continue (min xor max overflow, split/anti range).  Test
> suite on s390x has no regressions, bootstrap is ok, x86 running.

+          (if (TREE_CODE (type) == INTEGER_TYPE
+               && TYPE_PRECISION (type) > TYPE_PRECISION (TREE_TYPE (@3)))
+           (with

use INTEGRAL_TYPE_P.

+             bool ovf_undef = TYPE_OVERFLOW_UNDEFINED (inner_type);
+

so this is overflow behavior of the inner op.

+             /* Convert combined constant to tree of outer type if
+                there was no overflow in the original operation.  */

"in the original inner operation."

you then go on and use ovf_undef also for the outer operation:

+             if (ovf_undef || vr_outer == VR_RANGE)
+             {

but you do not actually _use_ vr_outer.  Do you think that if
vr_outer is a VR_RANGE then the outer operation may not
possibly have wrapped?  That's a false conclusion.

But I don't see how overflow in the original outer operation matters
and the code lacks comments as to explaining that as well.

So if you have a vr0 then you can compute whether the inner
operation cannot overflow.  You do this here:

+                   if (!ovf_undef && vr0 == VR_RANGE)
+                     {
+                       int max_ovf = 0;
+                       int min_ovf = 0;
+
+                       signop sgn = TYPE_SIGN (inner_type);
+
+                       wmin = wi::add (wmin0, w1);
+                       min_ovf = wi::cmp (wmin, w1, sgn) < 0;
+
+                       wmax = wi::add (wmax0, w1);
+                       max_ovf = wi::cmp (wmax, w1, sgn) < 0;
+
+                       ovf = min_ovf || max_ovf;
+
+                       split_range = ((min_ovf && !max_ovf)
+                                      || (!min_ovf && max_ovf));

ah, here's the use of the outer value-range.  This lacks a comment
(and it looks fishy given the outer value-range is a conservative approximation
and thus could be [-INF, +INF]).  Why's this not using the
wi::add overload with the overflow flag?  ISTR you want to handle "negative"
unsigned constants somehow, but then I don't see how the above works.
I'd say if wmin/wmax interpreted as signed are positive and then using
a signed op to add w1 results in a still positive number you're fine
(you don't seem
to restrict the widening cast to either zero- or sign-extending).

+                   if (ovf_undef || !split_range)
+                     {
+                       /* Extend @1 to TYPE. */
+                       w1 = w1.from (w1, TYPE_PRECISION (type),
+                                     ovf ? SIGNED : TYPE_SIGN
(TREE_TYPE (@1)));

ideally you could always interpret w1 as signed?

+                       /* Combine in outer, larger type.  */
+                       wide_int combined_cst;
+                       combined_cst = wi::add (w1, w2);

+(if (cst)
+        (outer_op (convert @0) { cst; }))
+       )))))

bogus indent.

+/* ((T)(A)) +- CST -> (T)(A +- CST)  */
+#if GIMPLE
+   (for outer_op (plus minus)
+    (simplify
+     (outer_op (convert SSA_NAME@0) INTEGER_CST@2)
+      (if (TYPE_PRECISION (type) > TYPE_PRECISION (TREE_TYPE (@0))
+          && TREE_CODE (TREE_TYPE (@0)) == INTEGER_TYPE
+          && TREE_CODE (type) == INTEGER_TYPE)

INTEGRAL_TYPE_P and do that first before looking at TYPE_PRECISION.

+           if (vr == VR_RANGE)
+             {
+               wide_int wmin = wi::add (wmin0, w1);
+               bool min_ovf = wi::cmp (wmin, w1, sgn) < 0;
+
+               wide_int wmax = wi::add (wmax0, w1);
+               bool max_ovf = wi::cmp (wmax, w1, sgn) < 0;
+
+               split_range = (min_ovf && !max_ovf) || (!min_ovf && max_ovf);

similar why not use wi:add overload with the overflow flag?

Btw, I find

  (with
   {
     tree x = NULL;
      if (...) x = non-NULL;
   }
   (if (x)
    (....))))

ugly.  Use

   (with
    {
      ...
    }
    (if (...)
     (... { non-NULL } )

or sth like that which makes control flow more easily visible.

Richard.


> Regards
>  Robin
>
> --
>
> gcc/ChangeLog:
>
> 2017-06-19  Robin Dapp  <rdapp@linux.vnet.ibm.com>
>
>         * match.pd: Simplify wrapped binary operations.