This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH]Fix computation of offset in ivopt
- From: Richard Biener <richard dot guenther at gmail dot com>
- To: "Bin.Cheng" <amker dot cheng at gmail dot com>
- Cc: "bin.cheng" <bin dot cheng at arm dot com>, GCC Patches <gcc-patches at gcc dot gnu dot org>
- Date: Wed, 2 Oct 2013 10:32:07 +0200
- Subject: Re: [PATCH]Fix computation of offset in ivopt
- Authentication-results: sourceware.org; auth=none
- References: <001501ceb906$56da1c00$048e5400$ at arm dot com> <CAFiYyc0WO3M8x-0eyN4xpami9yemcX0cVy=awfOFYYmeo+OSyg at mail dot gmail dot com> <002701cebb3f$6fcd6ab0$4f684010$ at arm dot com> <CAFiYyc0M_7mSxED5YK7WYy+7mxd3Axdzdy=smnSEQj2iPx9m3A at mail dot gmail dot com> <002901cebd9f$5f7cb0a0$1e7611e0$ at arm dot com> <CAFiYyc1J1FBhsgsj9hgcMWg4o8M7soY3h4CGQWa+FyVqrEv_eQ at mail dot gmail dot com> <CAHFci295kKk7CXiEPfEGiPDy+ekew7TgEWygr-_prYbV9Sscow at mail dot gmail dot com>
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