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