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


On Fri, Oct 18, 2013 at 5:48 PM, Bin.Cheng <amker.cheng@gmail.com> wrote:
> Hi Richard,
> Is the first patch OK?  Since there is a patch depending on it.
> http://gcc.gnu.org/ml/gcc-patches/2013-09/msg01931.html

Yes.

Richard.

> Thanks.
> bin
>
> On Fri, Oct 18, 2013 at 7:18 PM, Richard Biener
> <richard.guenther@gmail.com> wrote:
>> On Thu, Oct 17, 2013 at 7:52 AM, bin.cheng <bin.cheng@arm.com> wrote:
>>> 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.
>>
>> Index: gcc/fold-const.c
>> ===================================================================
>> --- gcc/fold-const.c (revision 203267)
>> +++ gcc/fold-const.c (working copy)
>> @@ -7270,8 +7270,8 @@ fold_plusminus_mult_expr (location_t loc, enum tre
>>        HOST_WIDE_INT int01, int11, tmp;
>>        bool swap = false;
>>        tree maybe_same;
>> -      int01 = TREE_INT_CST_LOW (arg01);
>> -      int11 = TREE_INT_CST_LOW (arg11);
>> +      int01 = int_cst_value (arg01);
>> +      int11 = int_cst_value (arg11);
>>
>> this is not correct - it will mishandle all large unsigned numbers.
>>
>> The real issue is that we rely on twos-complement arithmetic to work
>> when operating on pointer offsets (because we convert them all to
>> unsigned sizetype).  That makes interpreting the offsets or expressions
>> that compute offsets hard (or impossible in some cases), as you can
>> see from the issues in IVOPTs.
>>
>> The above means that strip_offset_1 cannot reliably look through
>> MULT_EXPRs as that can make an offset X * CST that is
>> "effectively" signed "surely" unsigned in the process.  Think of
>> this looking into X * CST as performing a division - whose result
>> is dependent on the sign of X which we lost with our unsigned
>> casting.  Now, removing the MULT_EXPR look-through might
>> be too pessimizing ... but it may be worth trying to see if it fixes
>> your issue?  In this context I also remember a new bug filed
>> recently of us not folding x /[ex] 4 * 4 to x.
>>
>> In the past I was working multiple times on stopping doing that
>> (make all offsets unsigned), but I never managed to finish ...
>>
>> Richard.
>>
>>> 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
>
>
>
> --
> Best Regards.


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