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
