On Mon, Oct 21, 2013 at 8:21 PM, Richard Biener <richard.guent...@gmail.com> wrote: > On Fri, Oct 18, 2013 at 5:48 PM, Bin.Cheng <amker.ch...@gmail.com> wrote: >> Hi Richard, >> Is the first patch OK? Since there is a patch depending on it. >> http://gcc.gnu.org/ml/gcc-patches/2013-09/msg01931.html > > Yes. I committed the patch fixing strip_offset_1 as r204116. Since the root cause of the second issue is POINTER_PLUS_EXPR requires an unsigned offset operand and can't be fixed as in the second patch, I will discard that patch.
Thanks. bin >> On Fri, Oct 18, 2013 at 7:18 PM, Richard Biener >> <richard.guent...@gmail.com> wrote: >>> On Thu, Oct 17, 2013 at 7:52 AM, bin.cheng <bin.ch...@arm.com> wrote: >>>> Hi, >>>> As noted in previous messages, GCC forces offset to unsigned in middle end. >>>> It also gets offset value and stores it in HOST_WIDE_INT then uses it in >>>> various computation. In this scenario, It should use int_cst_value to do >>>> additional sign extension against the type of tree node, otherwise we might >>>> lose sign information. Take function fold_plusminus_mult_expr as an >>>> example, the sign information of arg01/arg11 might be lost because it uses >>>> TREE_INT_CST_LOW directly. I think this is the offender of the problem in >>>> this thread. I think we should fix the offender, rather the hacking >>>> strip_offset as in the original patch, so I split original patch into two >>>> as >>>> attached, with one fixing the offset of COMPONENT_REF in strip_offset_1, >>>> the >>>> other fixing the mentioned sign extension problem. >>> >>> Index: gcc/fold-const.c >>> =================================================================== >>> --- gcc/fold-const.c (revision 203267) >>> +++ gcc/fold-const.c (working copy) >>> @@ -7270,8 +7270,8 @@ fold_plusminus_mult_expr (location_t loc, enum tre >>> HOST_WIDE_INT int01, int11, tmp; >>> bool swap = false; >>> tree maybe_same; >>> - int01 = TREE_INT_CST_LOW (arg01); >>> - int11 = TREE_INT_CST_LOW (arg11); >>> + int01 = int_cst_value (arg01); >>> + int11 = int_cst_value (arg11); >>> >>> this is not correct - it will mishandle all large unsigned numbers. >>> >>> The real issue is that we rely on twos-complement arithmetic to work >>> when operating on pointer offsets (because we convert them all to >>> unsigned sizetype). That makes interpreting the offsets or expressions >>> that compute offsets hard (or impossible in some cases), as you can >>> see from the issues in IVOPTs. >>> >>> The above means that strip_offset_1 cannot reliably look through >>> MULT_EXPRs as that can make an offset X * CST that is >>> "effectively" signed "surely" unsigned in the process. Think of >>> this looking into X * CST as performing a division - whose result >>> is dependent on the sign of X which we lost with our unsigned >>> casting. Now, removing the MULT_EXPR look-through might >>> be too pessimizing ... but it may be worth trying to see if it fixes >>> your issue? In this context I also remember a new bug filed >>> recently of us not folding x /[ex] 4 * 4 to x. >>> >>> In the past I was working multiple times on stopping doing that >>> (make all offsets unsigned), but I never managed to finish ... >>> >>> Richard. >>> >>>> Bootstrap and test on x86/x86_64/arm. Is it OK? >>>> >>>> Thanks. >>>> bin >>>> >>>> Patch a: >>>> 2013-10-17 Bin Cheng <bin.ch...@arm.com> >>>> >>>> * tree-ssa-loop-ivopts.c (strip_offset_1): Change parameter type. >>>> Count DECL_FIELD_BIT_OFFSET when computing offset for >>>> COMPONENT_REF. >>>> >>>> Patch b: >>>> 2013-10-17 Bin Cheng <bin.ch...@arm.com> >>>> >>>> * fold-const.c (fold_plusminus_mult_expr): Use int_cst_value >>>> instead >>>> of TREE_INT_CST_LOW. >>>> >>>> gcc/testsuite/ChangeLog >>>> 2013-10-17 Bin Cheng <bin.ch...@arm.com> >>>> >>>> * gcc.dg/tree-ssa/ivopts-outside-loop-use-1.c: New test. >>>> >>>>> -----Original Message----- >>>>> From: Richard Biener [mailto:richard.guent...@gmail.com] >>>>> Sent: Wednesday, October 02, 2013 4:32 PM >>>>> To: Bin.Cheng >>>>> Cc: Bin Cheng; GCC Patches >>>>> Subject: Re: [PATCH]Fix computation of offset in ivopt >>>>> >>>>> 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 >> >> >> >> -- >> Best Regards. -- Best Regards.
