| Message ID | CAFiYyc34mD6Rx+JUmv8d=1UH3_eqgqoeLE7wE-qBX7XAye7+ow@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
On 10/18/2013 01:18 PM, Richard Biener wrote: > 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. I still have patches to keep pointer types in ivopts (using a new POINTER_PLUSV_EXPR). Would that help in this case? Last time I posted them they met an unenthusiastic reception so I've never bothered to repost them. Bernd
On Fri, Oct 18, 2013 at 2:02 PM, Bernd Schmidt <bernds@codesourcery.com> wrote: > On 10/18/2013 01:18 PM, Richard Biener wrote: > >> 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. > > I still have patches to keep pointer types in ivopts (using a new > POINTER_PLUSV_EXPR). Would that help in this case? Last time I posted > them they met an unenthusiastic reception so I've never bothered to > repost them. Can you point me to that patch? Or shortly elaborate on "keep pointer types in ivopts"? I think this issue is about decomposing offset computations into a constant and a variable part, which after foldings messed up the unsigned computation can result in "odd" constant offset parts. So it's rather because the offset operand of POINTER_PLUS_EXPR is fixed as sizetype. Richard. > > Bernd >
On 10/18/2013 02:10 PM, Richard Biener wrote: > On Fri, Oct 18, 2013 at 2:02 PM, Bernd Schmidt <bernds@codesourcery.com> wrote: >> On 10/18/2013 01:18 PM, Richard Biener wrote: >> >>> 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. >> >> I still have patches to keep pointer types in ivopts (using a new >> POINTER_PLUSV_EXPR). Would that help in this case? Last time I posted >> them they met an unenthusiastic reception so I've never bothered to >> repost them. > > Can you point me to that patch? Or shortly elaborate on "keep pointer types > in ivopts"? I think this issue is about decomposing offset computations into > a constant and a variable part, which after foldings messed up the unsigned > computation can result in "odd" constant offset parts. So it's rather > because the offset operand of POINTER_PLUS_EXPR is fixed as > sizetype. Okay, my patch doesn't address that part, it only ensures the pointer base values are kept and arithmetic on them is done using POINTER_PLUS. The original patch was here. http://gcc.gnu.org/ml/gcc-patches/2012-03/msg01104.html Bernd
On Fri, Oct 18, 2013 at 2:26 PM, Bernd Schmidt <bernds@codesourcery.com> wrote: > On 10/18/2013 02:10 PM, Richard Biener wrote: >> On Fri, Oct 18, 2013 at 2:02 PM, Bernd Schmidt <bernds@codesourcery.com> wrote: >>> On 10/18/2013 01:18 PM, Richard Biener wrote: >>> >>>> 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. >>> >>> I still have patches to keep pointer types in ivopts (using a new >>> POINTER_PLUSV_EXPR). Would that help in this case? Last time I posted >>> them they met an unenthusiastic reception so I've never bothered to >>> repost them. >> >> Can you point me to that patch? Or shortly elaborate on "keep pointer types >> in ivopts"? I think this issue is about decomposing offset computations into >> a constant and a variable part, which after foldings messed up the unsigned >> computation can result in "odd" constant offset parts. So it's rather >> because the offset operand of POINTER_PLUS_EXPR is fixed as >> sizetype. > > Okay, my patch doesn't address that part, it only ensures the pointer > base values are kept and arithmetic on them is done using POINTER_PLUS. Ah, I recently fixed parts of that I think ... 2013-09-02 Richard Biener <rguenther@suse.de> * tree-affine.c (add_elt_to_tree): Avoid converting all pointer arithmetic to sizetype. maybe you can double-check if the result is what you desired ;) Richard. > The original patch was here. > http://gcc.gnu.org/ml/gcc-patches/2012-03/msg01104.html > > > Bernd >
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 Thanks. bin On Fri, Oct 18, 2013 at 7:18 PM, Richard Biener <richard.guenther@gmail.com> wrote: > On Thu, Oct 17, 2013 at 7:52 AM, bin.cheng <bin.cheng@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.cheng@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.cheng@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.cheng@arm.com> >> >> * gcc.dg/tree-ssa/ivopts-outside-loop-use-1.c: New test. >> >>> -----Original Message----- >>> From: Richard Biener [mailto:richard.guenther@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.cheng@gmail.com> wrote: >>> > On Tue, Oct 1, 2013 at 6:50 PM, Richard Biener >>> > <richard.guenther@gmail.com> wrote: >>> >> On Mon, Sep 30, 2013 at 7:39 AM, bin.cheng <bin.cheng@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
On Fri, Oct 18, 2013 at 7:18 PM, Richard Biener <richard.guenther@gmail.com> wrote: > On Thu, Oct 17, 2013 at 7:52 AM, bin.cheng <bin.cheng@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. As far as the patch itself. I think the preceding host_integerp already checks for this case: int host_integerp (const_tree t, int pos) { if (t == NULL_TREE) return 0; return (TREE_CODE (t) == INTEGER_CST && ((TREE_INT_CST_HIGH (t) == 0 && (HOST_WIDE_INT) TREE_INT_CST_LOW (t) >= 0) || (! pos && TREE_INT_CST_HIGH (t) == -1 && (HOST_WIDE_INT) TREE_INT_CST_LOW (t) < 0 && !TYPE_UNSIGNED (TREE_TYPE (t))) || (pos && TREE_INT_CST_HIGH (t) == 0))); } Since the call is host_integerp (xxx, 0), it returns 0 for large unsigned numbers, right? > > 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.cheng@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.cheng@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.cheng@arm.com> >> >> * gcc.dg/tree-ssa/ivopts-outside-loop-use-1.c: New test. >> >>> -----Original Message----- >>> From: Richard Biener [mailto:richard.guenther@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.cheng@gmail.com> wrote: >>> > On Tue, Oct 1, 2013 at 6:50 PM, Richard Biener >>> > <richard.guenther@gmail.com> wrote: >>> >> On Mon, Sep 30, 2013 at 7:39 AM, bin.cheng <bin.cheng@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
On Fri, Oct 18, 2013 at 5:48 PM, Bin.Cheng <amker.cheng@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.guenther@gmail.com> wrote: >> On Thu, Oct 17, 2013 at 7:52 AM, bin.cheng <bin.cheng@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.cheng@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.cheng@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.cheng@arm.com> >>> >>> * gcc.dg/tree-ssa/ivopts-outside-loop-use-1.c: New test. >>> >>>> -----Original Message----- >>>> From: Richard Biener [mailto:richard.guenther@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.cheng@gmail.com> wrote: >>>> > On Tue, Oct 1, 2013 at 6:50 PM, Richard Biener >>>> > <richard.guenther@gmail.com> wrote: >>>> >> On Mon, Sep 30, 2013 at 7:39 AM, bin.cheng <bin.cheng@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.
On Mon, Oct 21, 2013 at 8:21 PM, Richard Biener <richard.guenther@gmail.com> wrote: > On Fri, Oct 18, 2013 at 5:48 PM, Bin.Cheng <amker.cheng@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.guenther@gmail.com> wrote: >>> On Thu, Oct 17, 2013 at 7:52 AM, bin.cheng <bin.cheng@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.cheng@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.cheng@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.cheng@arm.com> >>>> >>>> * gcc.dg/tree-ssa/ivopts-outside-loop-use-1.c: New test. >>>> >>>>> -----Original Message----- >>>>> From: Richard Biener [mailto:richard.guenther@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.cheng@gmail.com> wrote: >>>>> > On Tue, Oct 1, 2013 at 6:50 PM, Richard Biener >>>>> > <richard.guenther@gmail.com> wrote: >>>>> >> On Mon, Sep 30, 2013 at 7:39 AM, bin.cheng <bin.cheng@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.
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.