On Tue, Sep 24, 2013 at 11:13 AM, bin.cheng <bin.ch...@arm.com> wrote:
> Hi,
> This patch fix two minor bugs when computing offset in IVOPT.
> 1) Considering below example:
> #define MAX 100
> struct tag
> {
> int i;
> int j;
> }
> struct tag arr[MAX]
>
> int foo (int len)
> {
> int i = 0;
> for (; i < len; i++)
> {
> access arr[i].j;
> }
> }
>
> Without this patch, the offset computed by strip_offset_1 for address
> arr[i].j is ZERO, which is apparently not.
>
> 2) Considering below example:
> //...
> <bb 15>:
> KeyIndex_66 = KeyIndex_194 + 4294967295;
> if (KeyIndex_66 != 0)
> goto <bb 16>;
> else
> goto <bb 18>;
>
> <bb 16>:
>
> <bb 17>:
> # KeyIndex_194 = PHI <KeyIndex_66(16), KeyIndex_180(73)>
> _62 = KeyIndex_194 + 1073741823;
> _63 = _62 * 4;
> _64 = pretmp_184 + _63;
> _65 = *_64;
> if (_65 == 0)
> goto <bb 15>;
> else
> goto <bb 71>;
> //...
>
> There are iv use and candidate like:
>
> use 1
> address
> in statement _65 = *_64;
>
> at position *_64
> type handletype *
> base pretmp_184 + ((sizetype) KeyIndex_180 + 1073741823) * 4
> step 4294967292
> base object (void *) pretmp_184
> related candidates
>
> candidate 6
> var_before ivtmp.16
> var_after ivtmp.16
> incremented before use 1
> type unsigned int
> base (unsigned int) (pretmp_184 + (sizetype) KeyIndex_180 * 4)
> step 4294967292
> base object (void *) pretmp_184
> Candidate 6 is related to use 1
>
> In function get_computation_cost_at for use 1 using cand 6, ubase and cbase
> are:
> pretmp_184 + ((sizetype) KeyIndex_180 + 1073741823) * 4
> pretmp_184 + (sizetype) KeyIndex_180 * 4
>
> The cstepi computed in HOST_WIDE_INT is : 0xfffffffffffffffc, while offset
> computed in TYPE(utype) is : 0xfffffffc. Though they both stand for value
> "-4" in different precision, statement "offset -= ratio * cstepi" returns
> 0x100000000, which is wrong.
>
> Tested on x86_64 and arm. Is it OK?
+ 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)))
{
...
}
note that this doesn't really handle overflows correctly as
+ *offset = off0 + int_cst_value (tmp) + boffset / BITS_PER_UNIT;
may still overflow.
@@ -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).
Richard.
> Thanks.
> bin
>
>
> 2013-09-24 Bin Cheng <bin.ch...@arm.com>
>
> * tree-ssa-loop-ivopts.c (strip_offset_1): Count
> DECL_FIELD_BIT_OFFSET when computing offset for COMPONENT_REF.
> (get_computation_cost_at): Sign extend offset.
