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: patch to fix constant math -5th patch, rtl


On Tue, Apr 16, 2013 at 10:17 PM, Kenneth Zadeck
<zadeck@naturalbridge.com> wrote:
> Here is a refreshed version of the rtl changes for wide-int.   the only
> change from the previous versions is that the wide-int binary operations
> have been simplified to use the new wide-int binary templates.

Looking for from_rtx calls (to see where we get the mode/precision from) I
see for example

-         o = rtx_to_double_int (outer);
-         i = rtx_to_double_int (inner);
-
-         m = double_int::mask (width);
-         i &= m;
-         m = m.llshift (offset, HOST_BITS_PER_DOUBLE_INT);
-         i = i.llshift (offset, HOST_BITS_PER_DOUBLE_INT);
-         o = o.and_not (m) | i;
-
+
+         o = (wide_int::from_rtx (outer, GET_MODE (SET_DEST (temp)))
+              .insert (wide_int::from_rtx (inner, GET_MODE (dest)),
+                       offset, width));

where I'd rather have the original code preserved as much as possible
and not introduce a new primitive wide_int::insert for this.  The conversion
and review process will be much more error-prone if we do multiple
things at once (and it might keep the wide_int initial interface leaner).

Btw, the wide_int::insert implementation doesn't assert anything about
the inputs precision.  Instead it reads

+  if (start + width >= precision)
+    width = precision - start;
+
+  mask = shifted_mask (start, width, false, precision);
+  tmp = op0.lshift (start, 0, precision, NONE);
+  result = tmp & mask;
+
+  tmp = and_not (mask);
+  result = result | tmp;

which eventually ends up performing everything in target precision.  So
we don't really care about the mode or precision of inner.

Then I see

diff --git a/gcc/dwarf2out.h b/gcc/dwarf2out.h
index ad03a34..531a7c1 100644
@@ -180,6 +182,7 @@ typedef struct GTY(()) dw_val_struct {
       HOST_WIDE_INT GTY ((default)) val_int;
       unsigned HOST_WIDE_INT GTY ((tag
("dw_val_class_unsigned_const"))) val_unsigned;
       double_int GTY ((tag ("dw_val_class_const_double"))) val_double;
+      wide_int GTY ((tag ("dw_val_class_wide_int"))) val_wide;
       dw_vec_const GTY ((tag ("dw_val_class_vec"))) val_vec;
       struct dw_val_die_union
        {

ick.  That makes dw_val_struct really large ... (and thus dw_attr_struct).
You need to make this a pointer to a wide_int at least.

-/* Return a CONST_INT or CONST_DOUBLE corresponding to target reading
+/* Return a constant integer corresponding to target reading
    GET_MODE_BITSIZE (MODE) bits from string constant STR.  */

 static rtx
 c_readstr (const char *str, enum machine_mode mode)
 {
-  HOST_WIDE_INT c[2];
+  wide_int c;
...
-  return immed_double_const (c[0], c[1], mode);
+
+  c = wide_int::from_array (tmp, len, mode);
+  return immed_wide_int_const (c, mode);
 }

err - what's this good for?  It doesn't look necessary as part of the initial
wide-int conversion at least.  (please audit your patches for such cases)

@@ -4994,12 +4999,12 @@ expand_builtin_signbit (tree exp, rtx target)

   if (bitpos < GET_MODE_BITSIZE (rmode))
     {
-      double_int mask = double_int_zero.set_bit (bitpos);
+      wide_int mask = wide_int::set_bit_in_zero (bitpos, rmode);

       if (GET_MODE_SIZE (imode) > GET_MODE_SIZE (rmode))
        temp = gen_lowpart (rmode, temp);
       temp = expand_binop (rmode, and_optab, temp,
-                          immed_double_int_const (mask, rmode),
+                          immed_wide_int_const (mask, rmode),
                           NULL_RTX, 1, OPTAB_LIB_WIDEN);
     }
   else

Likewise.  I suppose you remove immed_double_int_const but I see no
reason to do that.  It just makes your patch larger than necessary.

[what was the reason again to have TARGET_SUPPORTS_WIDE_INT at all?
It's supposed to be a no-op conversion, right?]

@@ -95,38 +95,9 @@ plus_constant (enum machine_mode mode, rtx x,
HOST_WIDE_INT c)

   switch (code)
     {
-    case CONST_INT:
-      if (GET_MODE_BITSIZE (mode) > HOST_BITS_PER_WIDE_INT)
-       {
-         double_int di_x = double_int::from_shwi (INTVAL (x));
-         double_int di_c = double_int::from_shwi (c);
-
-         bool overflow;
-         double_int v = di_x.add_with_sign (di_c, false, &overflow);
-         if (overflow)
-           gcc_unreachable ();
-
-         return immed_double_int_const (v, VOIDmode);
-       }
-
-      return GEN_INT (INTVAL (x) + c);
-
-    case CONST_DOUBLE:
-      {
-       double_int di_x = double_int::from_pair (CONST_DOUBLE_HIGH (x),
-                                                CONST_DOUBLE_LOW (x));
-       double_int di_c = double_int::from_shwi (c);
-
-       bool overflow;
-       double_int v = di_x.add_with_sign (di_c, false, &overflow);
-       if (overflow)
-         /* Sorry, we have no way to represent overflows this wide.
-            To fix, add constant support wider than CONST_DOUBLE.  */
-         gcc_assert (GET_MODE_BITSIZE (mode) <= HOST_BITS_PER_DOUBLE_INT);
-
-       return immed_double_int_const (v, VOIDmode);
-      }
-
+    CASE_CONST_SCALAR_INT:
+      return immed_wide_int_const (wide_int::from_rtx (x, mode)
+                                  + wide_int::from_shwi (c, mode), mode);

you said you didn't want to convert CONST_INT to wide-int.  But the above
is certainly a lot less efficient than before - given your change to support
operator+ RTX even less efficient than possible.  The above also shows
three 'mode' arguments while one to immed_wide_int_const would be
enough (given it would truncate the arbitrary precision result from the
addition to modes precision).

That is, I see no reason to remove the CONST_INT case or the CONST_DOUBLE
case.  [why is the above not in any way guarded with TARGET_SUPPORTS_WIDE_INT?]

What happens with overflows in the wide-int case?  The double-int case
asserted that there is no overflow across 2 * hwi precision, the wide-int
case does not.  Still the wide-int case now truncates to 'mode' precision
while the CONST_DOUBLE case did not.

That's a change in behavior, no?  Effectively the code for CONST_INT
and CONST_DOUBLE did "arbitrary" precision arithmetic (up to the
precision they can encode) which wide-int changes.

Can we in such cases please to a preparatory patch and change the
CONST_INT/CONST_DOUBLE paths to do an explicit [sz]ext to
mode precision first?  What does wide-int do with VOIDmode mode inputs?
It seems to ICE on them for from_rtx and use garbage (0) for from_shwi.  Ugh.
Btw, plus_constant asserts that mode is either VOIDmode (I suppose semantically
do "arbitrary precision") or the same mode as the mode of x (I suppose
semantically do "mode precision").  Neither the current nor your implementation
seems to do something consistent here :/

So please, for those cases (I suppose there are many more, eventually
one of the reasons why you think that requiring a mode for all CONST_DOUBLEs
is impossible), can we massage the current code to 1) document what is
desired, 2) follow that specification with regarding to computation
mode / precision
and result mode / precision?

Thanks,
Richard.

> kenny
>


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