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.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
Index: gcc/tree-ssa-loop-ivopts.c
===================================================================
--- gcc/tree-ssa-loop-ivopts.c (revision 203267)
+++ gcc/tree-ssa-loop-ivopts.c (working copy)
@@ -2037,12 +2037,12 @@ find_interesting_uses (struct ivopts_data *data)
static tree
strip_offset_1 (tree expr, bool inside_addr, bool top_compref,
- unsigned HOST_WIDE_INT *offset)
+ HOST_WIDE_INT *offset)
{
tree op0 = NULL_TREE, op1 = NULL_TREE, tmp, step;
enum tree_code code;
tree type, orig_type = TREE_TYPE (expr);
- unsigned HOST_WIDE_INT off0, off1, st;
+ HOST_WIDE_INT off0, off1, st;
tree orig_expr = expr;
STRIP_NOPS (expr);
@@ -2133,19 +2133,32 @@ strip_offset_1 (tree expr, bool inside_addr, bool
break;
case COMPONENT_REF:
- if (!inside_addr)
- return orig_expr;
+ {
+ tree field;
- tmp = component_ref_field_offset (expr);
- if (top_compref
- && cst_and_fits_in_hwi (tmp))
- {
- /* Strip the component reference completely. */
- op0 = TREE_OPERAND (expr, 0);
- op0 = strip_offset_1 (op0, inside_addr, top_compref, &off0);
- *offset = off0 + int_cst_value (tmp);
- return op0;
- }
+ if (!inside_addr)
+ return orig_expr;
+
+ tmp = component_ref_field_offset (expr);
+ field = TREE_OPERAND (expr, 1);
+ if (top_compref
+ && cst_and_fits_in_hwi (tmp)
+ && cst_and_fits_in_hwi (DECL_FIELD_BIT_OFFSET (field)))
+ {
+ HOST_WIDE_INT boffset, abs_off;
+
+ /* Strip the component reference completely. */
+ op0 = TREE_OPERAND (expr, 0);
+ op0 = strip_offset_1 (op0, inside_addr, top_compref, &off0);
+ boffset = int_cst_value (DECL_FIELD_BIT_OFFSET (field));
+ abs_off = abs_hwi (boffset) / BITS_PER_UNIT;
+ if (boffset < 0)
+ abs_off = -abs_off;
+
+ *offset = off0 + int_cst_value (tmp) + abs_off;
+ return op0;
+ }
+ }
break;
case ADDR_EXPR:
@@ -2196,7 +2209,10 @@ strip_offset_1 (tree expr, bool inside_addr, bool
static tree
strip_offset (tree expr, unsigned HOST_WIDE_INT *offset)
{
- return strip_offset_1 (expr, false, false, offset);
+ HOST_WIDE_INT off;
+ tree core = strip_offset_1 (expr, false, false, &off);
+ *offset = off;
+ return core;
}
/* Returns variant of TYPE that can be used as base for different uses.
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);
/* Move min of absolute values to int11. */
if (absu_hwi (int01) < absu_hwi (int11))
Index: gcc/testsuite/gcc.dg/fold-pointer_plus_expr-offset.c
===================================================================
--- gcc/testsuite/gcc.dg/fold-pointer_plus_expr-offset.c (revision 0)
+++ gcc/testsuite/gcc.dg/fold-pointer_plus_expr-offset.c (revision 0)
@@ -0,0 +1,13 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -fdump-tree-gimple" } */
+
+int foo(int *a, int l)
+{
+ while (l && ! a[l-1])
+ --l;
+
+ return l;
+}
+
+/* { dg-final { scan-tree-dump-not "1073741823" "gimple" } } */
+/* { dg-final { cleanup-tree-dump "gimple" } } */
