On Mon, Sep 30, 2013 at 7:39 AM, bin.cheng <bin.ch...@arm.com> wrote:
>
>
>> -----Original Message-----
>> From: Richard Biener [mailto:richard.guent...@gmail.com]
>> Sent: Friday, September 27, 2013 4:30 PM
>> To: Bin Cheng
>> Cc: GCC Patches
>> Subject: Re: [PATCH]Fix computation of offset in ivopt
>>
>> On Fri, Sep 27, 2013 at 7:07 AM, bin.cheng <bin.ch...@arm.com> wrote:
>> >
>> >
>> > case INTEGER_CST:
>> > //.......
>> > *offset = int_cst_value (expr);
>> > change to
>> > case INTEGER_CST:
>> > //.......
>> > *offset = sext_hwi (int_cst_value (expr), type);
>> >
>> > and
>> > case MULT_EXPR:
>> > //.......
>> > *offset = sext_hwi (int_cst_value (expr), type); to
>> > case MULT_EXPR:
>> > //.......
>> > HOST_WIDE_INT xxx = (HOST_WIDE_INT)off0 * int_cst_value (op1);
>> > *offset = sext_hwi (xxx, type);
>> >
>> > Any comments?
>>
>> The issue is of course that we end up converting offsets to sizetype at
> some
>> point which makes them all appear unsigned. The fix for this is to simply
>> interpret them as signed ... but it's really a mess ;)
>>
>
> Hi, this is updated patch which calculates signed offset in strip_offset_1
> then sign extend it in strip_offset.
>
> Bootstrap and test on x86_64/x86/arm. Is it OK?
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 ...
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.
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.
Btw, int_cst_value sounds like a very bad name for a value-changing
function.
Richard.
> Thanks.
> bin
>
> 2013-09-30 Bin Cheng <bin.ch...@arm.com>
>
> * tree-ssa-loop-ivopts.c (strip_offset_1): Change parameter type.
> Count DECL_FIELD_BIT_OFFSET when computing offset for COMPONENT_REF.
> (strip_offset): Sign extend before return.
