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. Richard. > 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.
