This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |
Other format: | [Raw text] |
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. 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
Attachment:
1-ivopt-offset-20131010-a.txt
Description: Text document
Attachment:
1-ivopt-offset-20131010-b.txt
Description: Text document
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |