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: wide-int branch now up for public comment and review


On Wed, 28 Aug 2013, Richard Sandiford wrote:

> Richard Biener <rguenther@suse.de> writes:
> >> * As above, constants coming from rtl are already in the right form,
> >>   so if you create a wide_int from an rtx and only query it, no explicit
> >>   extension is needed.
> >> 
> >> * Things like logical operations and right shifts naturally preserve
> >>   the sign-extended form, so only a subset of write operations need
> >>   to take special measures.
> >> 
> >> * You have a public interface that exposes the underlying HWIs
> >>   (which is fine with me FWIW), so it seems better to expose a fully-defined
> >>   HWI rather than only a partially-defined HWI.
> >> 
> >> E.g. zero_p is:
> >> 
> >>   HOST_WIDE_INT x;
> >> 
> >>   if (precision && precision < HOST_BITS_PER_WIDE_INT)
> >>     x = sext_hwi (val[0], precision);
> >>   else if (len == 0)
> >>     {
> >>       gcc_assert (precision == 0);
> >>       return true;
> >>     }
> >>   else
> >>     x = val[0];
> >> 
> >>   return len == 1 && x == 0;
> >> 
> >> but I think it really ought to be just:
> >> 
> >>   return len == 1 && val[0] == 0;
> >
> > Yes!
> >
> > But then - what value does keeping track of a 'precision' have
> > in this case?  It seems to me it's only a "convenient carrier"
> > for
> >
> >   wide_int x = wide-int-from-RTX (y);
> >   machine_mode saved_mode = mode-available? GET_MODE (y) : magic-mode;
> >   ... process x ...
> >   RTX = RTX-from-wide_int (x, saved_mode);
> >
> > that is, wide-int doesn't do anything with 'precision' but you
> > can extract it later to not need to remember a mode you were
> > interested in?
> 
> I can see why you like the constant-precision, very wide integers for trees,
> where the constants have an inherent sign.  But (and I think this might be
> old ground too :-)), that isn't the case with rtl.  At the tree level,
> using constant-precision, very wide integers allows you to add a 32-bit
> signed INTEGER_CST to a 16-unsigned INTEGER_CST.  And that has an
> obvious meaning, both as a 32-bit result or as a wider result, depending
> on how you choose to use it.  But in rtl there is no meaning to adding
> an SImode and an HImode value together, since we don't know how to
> extend the HImode value beyond its precision.  You must explicitly sign-
> or zero-extend the value first.  (The fact that we choose to sign-extend
> rtl constants when storing them in HWIs is just a representation detail,
> to avoid having undefined bits in the HWIs.  It doesn't mean that rtx
> values themselves are signed.  We could have used a zero-extending
> representation instead without changing the semantics.)

Yeah, that was my understanding.

> So the precision variable is good for the rtl level in several ways:
> 
> - As you say, it avoids adding the explicit truncations that (in practice)
>   every rtl operation would need
> 
> - It's more efficient in that case, since we don't calculate high values
>   and then discard them immediately.  The common GET_MODE_PRECISION (mode)
>   <= HOST_BITS_PER_WIDE_INT case stays a pure HWI operation, despite all
>   the wide-int trappings.
> 
> - It's a good way of checking type safety and making sure that excess
>   bits aren't accidentally given a semantic meaning.  This is the most
>   important reason IMO.
> 
> The branch has both the constant-precision, very wide integers that we
> want for trees and the variable-precision integers we want for rtl,
> so it's not an "either or".  With the accessor-based implementation,
> there should be very little cost to having both.

So what I wonder (and where we maybe disagree) is how much code
wants to inspect "intermediate" results.  Say originally you have

rtx foo (rtx x, rtx y)
{
  rtx tem = simplify_const_binary_operation (PLUS, GET_MODE (x), x, 
GEN_INT (1));
  rtx res = simplify_const_binary_operation (MINUS, GET_MODE (tem), tem, 
y);
  return res;
}

and with wide-int you want to change that to

rtx foo (rtx x, rtx y)
{
  wide_int tem = wide_int (x) + 1;
  wide_int res = tem - y;
  return res.to_rtx ();
}

how much code ever wants to inspect 'tem' or 'res'?
That is, does it matter
if 'tem' and 'res' would have been calculated in "infinite precision"
and only to_rtx () would do the truncation to the desired mode?

I think not.  The amount of code performing multiple operations on
_constants_ in sequence is extremely low (if it even exists).

So I'd rather have to_rtx get a mode argument (or a precision) and
perform the required truncation / sign-extension at RTX construction
time (which is an expensive operation anyway).

So where does this "break"?  It only breaks where previous code
broke (or where previous code had measures to not break that we
don't carry over to the wide-int case).  Obvious case is unsigned
division on the sign-extended rep of RTL constants for example.

> >> The main thing that's changed since the early patches is that we now
> >> have a mixture of wide-int types.  This seems to have led to a lot of
> >> boiler-plate forwarding functions (or at least it felt like that while
> >> moving them all out the class).  And that in turn seems to be because
> >> you're trying to keep everything as member functions.  E.g. a lot of the
> >> forwarders are from a member function to a static function.
> >> 
> >> Wouldn't it be better to have the actual classes be light-weight,
> >> with little more than accessors, and do the actual work with non-member
> >> template functions?  There seems to be 3 grades of wide-int:
> >> 
> >>   (1) read-only, constant precision  (from int, etc.)
> >>   (2) read-write, constant precision  (fixed_wide_int)
> >>   (3) read-write, variable precision  (wide_int proper)
> >> 
> >> but we should be able to hide that behind templates, with compiler errors
> >> if you try to write to (1), etc.
> >
> > Yeah, I'm probably trying to clean up the implementation once I
> > got past recovering from two month without GCC ...
> 
> FWIW, I've been plugging away at a version that uses accessors.
> I hope to have it vaguely presentable by the middle of next week,
> in case your recovery takes that long...

Depends on my priorities ;)

Btw, rtl.h still wastes space with

struct GTY((variable_size)) hwivec_def {
  int num_elem;         /* number of elements */
  HOST_WIDE_INT elem[1];
};

struct GTY((chain_next ("RTX_NEXT (&%h)"),
            chain_prev ("RTX_PREV (&%h)"), variable_size)) rtx_def {
...
  /* The first element of the operands of this rtx.
     The number of operands and their types are controlled
     by the `code' field, according to rtl.def.  */
  union u {
    rtunion fld[1];
    HOST_WIDE_INT hwint[1];
    struct block_symbol block_sym;
    struct real_value rv;
    struct fixed_value fv;
    struct hwivec_def hwiv;
  } GTY ((special ("rtx_def"), desc ("GET_CODE (&%0)"))) u;
};

there are 32bits available before the union.  If you don't use
those for num_elem then all wide-ints will at least take as
much space as DOUBLE_INTs originally took - and large ints
that would have required DOUBLE_INTs in the past will now
require more space than before.  Which means your math
motivating the 'num_elem' encoding stuff is wrong.  With
moving 'num_elem' before u you can even re-use the hwint
field in the union as the existing double-int code does
(which in fact could simply do the encoding trick in the
old CONST_DOUBLE scheme, similar for the tree INTEGER_CST
container).

Richard.


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