On Tue, Oct 1, 2013 at 6:13 PM, Bin.Cheng <amker.ch...@gmail.com> wrote: > On Tue, Oct 1, 2013 at 6:50 PM, Richard Biener > <richard.guent...@gmail.com> wrote: >> On Mon, Sep 30, 2013 at 7:39 AM, bin.cheng <bin.ch...@arm.com> wrote: >>> >>> >> >> 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 ... > There is a problem for example of the first message. The iv base if like: > pretmp_184 + ((sizetype) KeyIndex_180 + 1073741823) * 4 > I am not sure why but it seems (-4/0xFFFFFFFC) is represented by > (1073741823*4). For each operand strip_offset_1 returns exactly the > positive number and result of multiplication never get its chance of > sign extend. That's why the sign extend is necessary to fix the > problem. Or it should be fixed elsewhere by representing iv base with: > "pretmp_184 + ((sizetype) KeyIndex_180 + 4294967295) * 4" in the first place.
Yeah, that's why I said the whole issue with forcing all offsets to be unsigned is a mess ... There is really no good answer besides not doing that I fear. Yes, in the above case we could fold the whole thing differently (interpret the offset of a POINTER_PLUS_EXPR as signed). You can try tracking down the offender, but it'll get non-trivial easily as you have to consider the fact that GCC will treat signed operations as having undefined behavior on overflow. So I see why you want to do the extension above (re-interpret the result), I suppose we can live with it but please make sure to add a big fat ??? comment before it explaining why it is necessary. Richard. >> >> 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. > So I can handle it like: > > + abs_boffset = abs_hwi (boffset); > + xxxxx = abs_boffset / BITS_PER_UNIT; > + if (boffset < 0) > + xxxxx = -xxxxx; > + *offset = off0 + int_cst_value (tmp) + xxxxx; > > Right? > >> >> 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. > See above. > > Thanks. > bin