This is the mail archive of the
gcc@gcc.gnu.org
mailing list for the GCC project.
Re: this code in fold-const.c:fold_single_bit_test looks wrong to me
- From: Richard Biener <richard dot guenther at gmail dot com>
- To: Joern Rennecke <joern dot rennecke at embecosm dot com>
- Cc: Kenneth Zadeck <zadeck at naturalbridge dot com>, GCC Development <gcc at gcc dot gnu dot org>, kenner at nyu dot edu
- Date: Fri, 30 Aug 2013 12:09:13 +0200
- Subject: Re: this code in fold-const.c:fold_single_bit_test looks wrong to me
- Authentication-results: sourceware.org; auth=none
- References: <51C99DDE dot 8090507 at naturalbridge dot com> <20130701075442 dot 74hswm52tc0wococ-nzlynne at webmail dot spamcop dot net>
On Mon, Jul 1, 2013 at 1:54 PM, Joern Rennecke
<joern.rennecke@embecosm.com> wrote:
> Quoting Kenneth Zadeck <zadeck@naturalbridge.com>:
>
>> if (TREE_CODE (inner) == RSHIFT_EXPR
>> && TREE_CODE (TREE_OPERAND (inner, 1)) == INTEGER_CST
>> && TREE_INT_CST_HIGH (TREE_OPERAND (inner, 1)) == 0
>> && bitnum < TYPE_PRECISION (type)
>> && 0 > compare_tree_int (TREE_OPERAND (inner, 1),
>> bitnum - TYPE_PRECISION (type)))
>> {
>> bitnum += TREE_INT_CST_LOW (TREE_OPERAND (inner, 1));
>> inner = TREE_OPERAND (inner, 0);
>> }
>>
>>
>> in particular, in the last stanza of the test
>> TREE_OPERAND (inner, 1) is a positive number from the second stanza.
>> bitnum is also always positive and less than the TYPE_PRECISION (type)
>> from the third stanza,
>> so bitnum - TYPE_PRECISION (type) is always negative,
>
>
> Not when you pass it as an "unsigned HOST_WIDE_INT", but then, this
> doesn't really make for sane code...
>
>
>> so the compare will always be positive, so this code will never be
>> executed.
>
>
> ... compare will almost always be negative, so this code will be executed,
> regardless of the validity of the shift or the bit test being in range.
>
>>
>> it is hard to believe that this is what you want.
>
>
> I see that this code lived previously in expr.c:store_flag_value,
> and was modified by a big omnibus patch there:
>
> Mon Mar 6 15:22:29 2000 Richard Kenner <kenner@vlsi1.ultra.nyu.edu>
> * expr.c
> ..
> (do_store_flag): Use compare_tree_int.
> ..
> @@ -10204,8 +10204,9 @@ do_store_flag (exp, target, mode, only_c
>
> if (TREE_CODE (inner) == RSHIFT_EXPR
> && TREE_CODE (TREE_OPERAND (inner, 1)) == INTEGER_CST
> && TREE_INT_CST_HIGH (TREE_OPERAND (inner, 1)) == 0
> - && (bitnum + TREE_INT_CST_LOW (TREE_OPERAND (inner, 1))
> - < TYPE_PRECISION (type)))
> + && bitnum < TYPE_PRECISION (type)
> + && 0 > compare_tree_int (TREE_OPERAND (inner, 1),
> + bitnum - TYPE_PRECISION (type)))
>
> {
> bitnum += TREE_INT_CST_LOW (TREE_OPERAND (inner, 1));
> inner = TREE_OPERAND (inner, 0);
>
> I suppose that should be "TYPE_PRECISION (type) - bitnum" instead.
Indeed. And more readable write it as
if (TREE_CODE (inner) == RSHIFT_EXPR
&& TREE_CODE (TREE_OPERAND (inner, 1)) == INTEGER_CST
&& host_integerp (TREE_OPERAND (inner, 1), 1)
&& bitnum < TYPE_PRECISION (type)
&& TREE_INT_CST_LOW (TREE_OPERAND (inner, 1)) <
TYPE_PRECISION (type) - bitnum)
testing that now.
Richard.