On Tue, Oct 1, 2013 at 6:50 PM, Richard Biener
<richard.guent...@gmail.com> wrote:
> On Mon, Sep 30, 2013 at 7:39 AM, bin.cheng <bin.ch...@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.
>
> 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
