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, Kenneth Zadeck wrote:

> 
> > > >    Note that the bits above the precision are not defined and the
> > > >    algorithms used here are careful not to depend on their value.  In
> > > >    particular, values that come in from rtx constants may have random
> > > >    bits.
> > Which is a red herring.  It should be fixed.  I cannot even believe
> > that sentence given the uses of CONST_DOUBLE_LOW/CONST_DOUBLE_HIGH
> > or INTVAL/UINTVAL.  I don't see accesses masking out 'undefined' bits
> > anywhere.
> > 
> I can agree with you that this could be fixed.   But it is not necessary to
> fix it.   The rtl level and most of the tree level has existed for a long time
> by doing math within the precision.
> 
> you do not see the masking at the rtl level because the masking is not
> necessary.    if you never look beyond the precision you just do not care.
> There is the issue with divide and mod and quite frankly the code on the trunk
> scares me to death.   my code at the rtl level makes sure that everything is
> clean since i can see if it is a udiv or a div that is enough info to clean
> the value up.
> 
> > > I have a feeling I'm rehashing a past debate, sorry, but rtx constants
> > > can't
> > > have random bits.  The upper bits must be a sign extension of the value.
> > > There's exactly one valid rtx for each (value, mode) pair.  If you saw
> > > something different then that sounds like a bug.  The rules should already
> > > be fairly well enforced though, since something like (const_int 128) --
> > > or (const_int 256) -- will not match a QImode operand.
> > See.  We're saved ;)
> this is richard's theory.   There is a block of code at wide-int.cc:175 that
> is ifdefed out that checks to see if the rtl consts are canonical.   if you
> bring it in, building the libraries fails very quickly.   The million dollar
> question is this the only bug or is this the first bug of a 1000 bugs.   Your
> comment about not seeing any masking cuts both ways.   There is very little
> code on the way in if you create ints with GEN_INT that makes sure someone has
> done the right thing on that side.   So my guess is that there are a lot of
> failures and a large number of them will be in the ports.

Well, clearly RTL code _expects_ constants to be properly extended.  I
can reproduce the bug and I simply guess that's a matter of mismatching
modes here (or performing an implicit truncation without properly
extending the constant).

    at /space/rguenther/src/svn/wide-int/gcc/combine.c:10086
10086                                                      GEN_INT 
(count));
(gdb) l
10081
10082                 mask_rtx = GEN_INT (nonzero_bits (varop, GET_MODE 
(varop)));
10083
10084                 mask_rtx
10085                   = simplify_const_binary_operation (code, 
result_mode, mask_rtx,
10086                                                      GEN_INT 
(count));

uses of GEN_INT are frowned upon ... for exactly that reason - the
mask_rtx is not a proper RTL constant for SImode.

Btw, all this isn't a reason to not have a well-defined wide-int
representation.  You just have (as you have for trees) to properly
canonize the representation at the time you convert from RTL
constants to wide-int constants.

In the long run we want a uniform representation of constants
so we can do zero-copying - but it looks like we now have
three different representations - the tree one (sign or zero
extended dependent on sign), RTL (garbage as you show) and
wide-int (always sign-extended).

That's why I was looking at at least matching what tree does
(because tree constants _are_ properly canonized).

Richard.


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