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]Fix computation of offset in ivopt


Hi,
As noted in previous messages, GCC forces offset to unsigned in middle end.
It also gets offset value and stores it in HOST_WIDE_INT then uses it in
various computation.  In this scenario, It should use int_cst_value to do
additional sign extension against the type of tree node, otherwise we might
lose sign information.  Take function fold_plusminus_mult_expr as an
example, the sign information of arg01/arg11 might be lost because it uses
TREE_INT_CST_LOW directly.  I think this is the offender of the problem in
this thread.  I think we should fix the offender, rather the hacking
strip_offset as in the original patch, so I split original patch into two as
attached, with one fixing the offset of COMPONENT_REF in strip_offset_1, the
other fixing the mentioned sign extension problem.

Bootstrap and test on x86/x86_64/arm.  Is it OK?

Thanks.
bin

Patch a:
2013-10-17  Bin Cheng  <bin.cheng@arm.com>

	* tree-ssa-loop-ivopts.c (strip_offset_1): Change parameter type.
	Count DECL_FIELD_BIT_OFFSET when computing offset for COMPONENT_REF.

Patch b: 
2013-10-17  Bin Cheng  <bin.cheng@arm.com>

	* fold-const.c (fold_plusminus_mult_expr): Use int_cst_value instead
	of TREE_INT_CST_LOW.

gcc/testsuite/ChangeLog
2013-10-17  Bin Cheng  <bin.cheng@arm.com>

	* gcc.dg/tree-ssa/ivopts-outside-loop-use-1.c: New test.

> -----Original Message-----
> From: Richard Biener [mailto:richard.guenther@gmail.com]
> Sent: Wednesday, October 02, 2013 4:32 PM
> To: Bin.Cheng
> Cc: Bin Cheng; GCC Patches
> Subject: Re: [PATCH]Fix computation of offset in ivopt
> 
> On Tue, Oct 1, 2013 at 6:13 PM, Bin.Cheng <amker.cheng@gmail.com> wrote:
> > On Tue, Oct 1, 2013 at 6:50 PM, Richard Biener
> > <richard.guenther@gmail.com> wrote:
> >> On Mon, Sep 30, 2013 at 7:39 AM, bin.cheng <bin.cheng@arm.com>
> wrote:
> >>>
> >>>
> >>
> >> I don't think you need
> >>
> >> +  /* Sign extend off if expr is in type which has lower precision
> >> +     than HOST_WIDE_INT.  */
> >> +  if (TYPE_PRECISION (TREE_TYPE (expr)) <= HOST_BITS_PER_WIDE_INT)
> >> +    off = sext_hwi (off, TYPE_PRECISION (TREE_TYPE (expr)));
> >>
> >> at least it would be suspicious if you did ...
> > There is a problem for example of the first message.  The iv base if
like:
> > pretmp_184 + ((sizetype) KeyIndex_180 + 1073741823) * 4 I am not sure
> > why but it seems (-4/0xFFFFFFFC) is represented by (1073741823*4).
> > For each operand strip_offset_1 returns exactly the positive number
> > and result of multiplication never get its chance of sign extend.
> > That's why the sign extend is necessary to fix the problem. Or it
> > should be fixed elsewhere by representing iv base with:
> > "pretmp_184 + ((sizetype) KeyIndex_180 + 4294967295) * 4" in the first
> place.
> 
> Yeah, that's why I said the whole issue with forcing all offsets to be
unsigned
> is a mess ...
> 
> There is really no good answer besides not doing that I fear.
> 
> Yes, in the above case we could fold the whole thing differently
(interpret
> the offset of a POINTER_PLUS_EXPR as signed).  You can try tracking down
> the offender, but it'll get non-trivial easily as you have to consider the
fact
> that GCC will treat signed operations as having undefined behavior on
> overflow.
> 
> So I see why you want to do the extension above (re-interpret the result),
I
> suppose we can live with it but please make sure to add a big fat ???
> comment before it explaining why it is necessary.
> 
> Richard.
> 
> >>
> >> The only case that I can think of points to a bug in strip_offset_1
> >> again, namely if sizetype (the type of all offsets) is smaller than a
> >> HOST_WIDE_INT in which case
> >>
> >> +    boffset = int_cst_value (DECL_FIELD_BIT_OFFSET (field));
> >> +    *offset = off0 + int_cst_value (tmp) + boffset / BITS_PER_UNIT;
> >>
> >> is wrong as boffset / BITS_PER_UNIT does not do a signed division
> >> then (for negative boffset which AFAIK does not happen - but it would
> >> be technically allowed).  Thus, the predicates like
> >>
> >> +    && cst_and_fits_in_hwi (tmp)
> >>
> >> would need to be amended with a check that the MSB is not set.
> > So I can handle it like:
> >
> > +    abs_boffset = abs_hwi (boffset);
> > +    xxxxx = abs_boffset / BITS_PER_UNIT;
> > +    if (boffset < 0)
> > +      xxxxx = -xxxxx;
> > +    *offset = off0 + int_cst_value (tmp) + xxxxx;
> >
> > Right?
> >
> >>
> >> Btw, the cst_and_fits_in_hwi implementation is odd:
> >>
> >> bool
> >> cst_and_fits_in_hwi (const_tree x)
> >> {
> >>   if (TREE_CODE (x) != INTEGER_CST)
> >>     return false;
> >>
> >>   if (TYPE_PRECISION (TREE_TYPE (x)) > HOST_BITS_PER_WIDE_INT)
> >>     return false;
> >>
> >>   return (TREE_INT_CST_HIGH (x) == 0
> >>           || TREE_INT_CST_HIGH (x) == -1); }
> >>
> >> the precision check seems totally pointless and I wonder what's the
> >> point of this routine as there is host_integerp () already and
> >> tree_low_cst instead of int_cst_value - oh, I see, the latter
> >> forcefully sign-extends .... that should make the extension not
> >> necessary.
> > See above.
> >
> > Thanks.
> > bin

Attachment: 1-ivopt-offset-20131010-a.txt
Description: Text document

Attachment: 1-ivopt-offset-20131010-b.txt
Description: Text document


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