On Tue, 2013-09-24 at 12:31 +0200, Richard Biener wrote: > 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). >
After reading "overflow" and "ivopt", I was wondering whether http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55190 is somehow related. Cheers, Oleg