RE: [PATCH]Fix computation of offset in ivopt

["bin.cheng" <bin dot cheng at arm dot com>]

> -----Original Message-----
> From: Richard Biener [mailto:richard.guenther@gmail.com]
> Sent: Tuesday, September 24, 2013 6:31 PM
> To: Bin Cheng
> Cc: GCC Patches
> Subject: Re: [PATCH]Fix computation of offset in ivopt
> 
> On Tue, Sep 24, 2013 at 11:13 AM, bin.cheng <bin.cheng@arm.com> wrote:
> 
> +       field = TREE_OPERAND (expr, 1);
> +       if (DECL_FIELD_BIT_OFFSET (field)
> +           && cst_and_fits_in_hwi (DECL_FIELD_BIT_OFFSET (field)))
> +         boffset = int_cst_value (DECL_FIELD_BIT_OFFSET (field));
> +
> +       tmp = component_ref_field_offset (expr);
> +       if (top_compref
> +           && cst_and_fits_in_hwi (tmp))
> +         {
> +           /* Strip the component reference completely.  */
> +           op0 = TREE_OPERAND (expr, 0);
> +           op0 = strip_offset_1 (op0, inside_addr, top_compref, &off0);
> +           *offset = off0 + int_cst_value (tmp) + boffset /
BITS_PER_UNIT;
> +           return op0;
> +         }
> 
> the failure paths seem mangled, that is, if cst_and_fits_in_hwi is false
for
> either offset part you may end up doing half accounting and not stripping.
> 
> Btw, DECL_FIELD_BIT_OFFSET is always non-NULL.  I suggest to rewrite to
> 
>      if (!inside_addr)
>        return orig_expr;
> 
>      tmp = component_ref_field_offset (expr);
>      field = TREE_OPERAND (expr, 1);
>      if (top_compref
>          && cst_and_fits_in_hwi (tmp)
>          && cst_and_fits_in_hwi (DECL_FIELD_BIT_OFFSET (field)))
>         {
>           ...
>         }
Will be refined.

> 
> note that this doesn't really handle overflows correctly as
> 
> +           *offset = off0 + int_cst_value (tmp) + boffset /
> + BITS_PER_UNIT;
> 
> may still overflow.
Since it's "unsigned + signed + signed", according to implicit conversion,
the signed operand will be converted to unsigned, so the overflow would only
happen when off0 is huge number and tmp/boffset is large positive number,
right?  Do I need to check whether off0 is larger than the overflowed
result?  Also there is signed->unsigned problem here, see below.

> 
> @@ -4133,6 +4142,9 @@ get_computation_cost_at (struct ivopts_data *data,
>          bitmap_clear (*depends_on);
>      }
> 
> +  /* Sign-extend offset if utype has lower precision than
> + HOST_WIDE_INT.  */  offset = sext_hwi (offset, TYPE_PRECISION
> + (utype));
> +
> 
> offset is computed elsewhere in difference_cost and the bug to me seems
> that it is unsigned.  sign-extending it here is odd at least (and the
extension
> should probably happen at sizetype precision, not that of utype).
I agree, The root cause is in split_offset_1, in which offset is computed.
Every time offset is computed in this function with a signed operand (like
"int_cst_value (tmp)" above), we need to take care the possible negative
number problem.   Take this case as an example, we need to do below change:

  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?

Thanks.
bin