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 Mon, Sep 30, 2013 at 7:39 AM, bin.cheng <bin.cheng@arm.com> wrote:
>
>
>> -----Original Message-----
>> From: Richard Biener [mailto:richard.guenther@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.cheng@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.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.
>         (strip_offset): Sign extend before return.


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