Re: Remove host_integerp tests with variable signedness

[Richard Biener <richard dot guenther at gmail dot com>]

On Fri, Nov 15, 2013 at 11:48 AM, Richard Sandiford
<rdsandiford@googlemail.com> wrote:
> Richard Biener <richard.guenther@gmail.com> writes:
>> On Thu, Nov 14, 2013 at 10:04 PM, Richard Sandiford
>> <rdsandiford@googlemail.com> wrote:
>>> Apart from the case in the C front end, there are 4 calls to host_integerp
>>> with a variable "pos" argument.  These "pos" arguments are all taken from
>>> TYPE_UNSIGNED.  In the dwarf2out.c case we go on to require:
>>>
>>>   simple_type_size_in_bits (TREE_TYPE (value)) <= HOST_BITS_PER_WIDE_INT
>>>   || host_integerp (value, 0)
>>>
>>> The host_integerp (value, 0) makes the first host_integerp trivially
>>> redundant for !TYPE_UNSIGNED.  Checking that the precision is
>>> <= HOST_BITS_PER_WIDE_INT makes the first host_integerp redundant
>>> for TYPE_UNSIGNED too, since all unsigned types of those precisions
>>> will fit in an unsigned HWI.  We already know that we're dealing with
>>> an INTEGER_CST, so there's no need for a code check either.
>>>
>>> vect_recog_divmod_pattern is similar in the sense that we already know
>>> that we have an INTEGER_CST and that we specifically check for precisions
>>> <= HOST_BITS_PER_WIDE_INT.
>>>
>>> In the other two cases we don't know whether we're dealing with an
>>> INTEGER_CST but we do check for precisions <= HOST_BITS_PER_WIDE_INT.
>>> So these host_integerps reduce to code tests.  (The precision check
>>> for expand_vector_divmod doesn't show up in the context but is at
>>> the top of the function:
>>>
>>>   if (prec > HOST_BITS_PER_WIDE_INT)
>>>     return NULL_TREE;
>>> )
>>>
>>> I also replaced the associated tree_low_csts with TREE_INT_CST_LOWs.
>>>
>>> Tested on x86_64-linux-gnu.  OK to install?
>>
>> Note that host_integerp "conveniently" also makes sure the argument
>> is not NULL and of INTEGER_CST kind, so without more context ....
>
> Right, which is why I was trying to list out the reasons why the
> INTEGER_CST check isn't needed in the first two cases.  None of the
> cases can be null AFAIK.
>
>>> Index: gcc/dwarf2out.c
>>> ===================================================================
>>> --- gcc/dwarf2out.c     2013-11-14 20:21:27.183058648 +0000
>>> +++ gcc/dwarf2out.c     2013-11-14 20:22:18.128829681 +0000
>>> @@ -17321,9 +17321,8 @@ gen_enumeration_type_die (tree type, dw_
>>>           if (TREE_CODE (value) == CONST_DECL)
>>>             value = DECL_INITIAL (value);
>>>
>>> -         if (host_integerp (value, TYPE_UNSIGNED (TREE_TYPE (value)))
>>> -             && (simple_type_size_in_bits (TREE_TYPE (value))
>>> -                 <= HOST_BITS_PER_WIDE_INT || host_integerp (value, 0)))
>>> +         if (simple_type_size_in_bits (TREE_TYPE (value))
>>> +             <= HOST_BITS_PER_WIDE_INT || host_integerp (value, 0))
>>
>> ... this isn't a 1:1 tranform
>
> The full context is:
>
>           if (simple_type_size_in_bits (TREE_TYPE (value))
>               <= HOST_BITS_PER_WIDE_INT || host_integerp (value, 0))
>             /* DWARF2 does not provide a way of indicating whether or
>                not enumeration constants are signed or unsigned.  GDB
>                always assumes the values are signed, so we output all
>                values as if they were signed.  That means that
>                enumeration constants with very large unsigned values
>                will appear to have negative values in the debugger.
>
>                TODO: the above comment is wrong, DWARF2 does provide
>                DW_FORM_sdata/DW_FORM_udata to represent signed/unsigned data.
>                This should be re-worked to use correct signed/unsigned
>                int/double tags for all cases, instead of always treating as
>                signed.  */
>             add_AT_int (enum_die, DW_AT_const_value, TREE_INT_CST_LOW (value));
>           else
>             /* Enumeration constants may be wider than HOST_WIDE_INT.  Handle
>                that here.  */
>             add_AT_double (enum_die, DW_AT_const_value,
>                            TREE_INT_CST_HIGH (value), TREE_INT_CST_LOW (value));
>
> We're dealing with a nonnull INTEGER_CST whatever happens.
>
>>> Index: gcc/tree-vect-patterns.c
>>> ===================================================================
>>> --- gcc/tree-vect-patterns.c    2013-11-14 20:21:27.183058648 +0000
>>> +++ gcc/tree-vect-patterns.c    2013-11-14 20:22:18.129829676 +0000
>>> @@ -2064,9 +2064,8 @@ vect_recog_divmod_pattern (vec<gimple> *
>>>        return pattern_stmt;
>>>      }
>>>
>>> -  if (!host_integerp (oprnd1, TYPE_UNSIGNED (itype))
>>> -      || integer_zerop (oprnd1)
>>> -      || prec > HOST_BITS_PER_WIDE_INT)
>>> +  if (prec > HOST_BITS_PER_WIDE_INT
>>> +      || integer_zerop (oprnd1))
>>
>> likewise.
>
> This is midway through a function that has already checked:
>
>   if (TREE_CODE (oprnd0) != SSA_NAME
>       || TREE_CODE (oprnd1) != INTEGER_CST
>       || TREE_CODE (itype) != INTEGER_TYPE
>       || TYPE_PRECISION (itype) != GET_MODE_PRECISION (TYPE_MODE (itype)))
>     return NULL;
>
> and where:
>
>   prec = TYPE_PRECISION (itype);
>
> So even without the host_integerp test, the code in the patch only fails
> to exit if we have an INTEGER_CST whose precision is <= HOST_BITS_PER_WIDE_INT.
> In that case the host_integerp test is redundant, because a unsigned constant
> will fit in an unsigned HWI and a signed constant will fit in a signed HWI.
>
>>> @@ -2078,8 +2077,8 @@ vect_recog_divmod_pattern (vec<gimple> *
>>>      {
>>>        unsigned HOST_WIDE_INT mh, ml;
>>>        int pre_shift, post_shift;
>>> -      unsigned HOST_WIDE_INT d = tree_low_cst (oprnd1, 1)
>>> -                                & GET_MODE_MASK (TYPE_MODE (itype));
>>
>> This ensures the value is positive ...
>
> This is in a:
>
>   if (TYPE_UNSIGNED (itype))
>     {
>
> block that is only reached if the conditions above are met.
>
>>> +      unsigned HOST_WIDE_INT d = (TREE_INT_CST_LOW (oprnd1)
>>> +                                 & GET_MODE_MASK (TYPE_MODE (itype)));
>>>        tree t1, t2, t3, t4;
>>>
>>>        if (d >= ((unsigned HOST_WIDE_INT) 1 << (prec - 1)))
>>> @@ -2195,7 +2194,7 @@ vect_recog_divmod_pattern (vec<gimple> *
>>>      {
>>>        unsigned HOST_WIDE_INT ml;
>>>        int post_shift;
>>> -      HOST_WIDE_INT d = tree_low_cst (oprnd1, 0);
>>> +      HOST_WIDE_INT d = TREE_INT_CST_LOW (oprnd1);
>>
>> While this also allows negatives.
>
> This is in the else (!TYPE_UNSIGNED) arm.
>
>>> Index: gcc/fold-const.c
>>> ===================================================================
>>> --- gcc/fold-const.c    2013-11-14 20:21:27.183058648 +0000
>>> +++ gcc/fold-const.c    2013-11-14 20:22:18.124829699 +0000
>>> @@ -12032,16 +12032,15 @@ fold_binary_loc (location_t loc,
>>>          if the new mask might be further optimized.  */
>>>        if ((TREE_CODE (arg0) == LSHIFT_EXPR
>>>            || TREE_CODE (arg0) == RSHIFT_EXPR)
>>> -         && host_integerp (TREE_OPERAND (arg0, 1), 1)
>>> -         && host_integerp (arg1, TYPE_UNSIGNED (TREE_TYPE (arg1)))
>>> -         && tree_low_cst (TREE_OPERAND (arg0, 1), 1)
>>> -            < TYPE_PRECISION (TREE_TYPE (arg0))
>>>           && TYPE_PRECISION (TREE_TYPE (arg0)) <= HOST_BITS_PER_WIDE_INT
>>> -         && tree_low_cst (TREE_OPERAND (arg0, 1), 1) > 0)
>>> +         && TREE_CODE (arg1) == INTEGER_CST
>>> +         && host_integerp (TREE_OPERAND (arg0, 1), 1)
>>> +         && tree_low_cst (TREE_OPERAND (arg0, 1), 1) > 0
>>> +         && (tree_low_cst (TREE_OPERAND (arg0, 1), 1)
>>> +             < TYPE_PRECISION (TREE_TYPE (arg0))))
>>>         {
>>>           unsigned int shiftc = tree_low_cst (TREE_OPERAND (arg0, 1), 1);
>>> -         unsigned HOST_WIDE_INT mask
>>> -           = tree_low_cst (arg1, TYPE_UNSIGNED (TREE_TYPE (arg1)));
>>> +         unsigned HOST_WIDE_INT mask = TREE_INT_CST_LOW (arg1);
>>
>> Where's the size test on arg1 gone?  IMHO this whole code should
>> just use double-ints ...
>
> Not sure which test you mean.  The only arg1 test is the host_integerp one,
> which I'm trying to get rid of.  The precision of arg0 is the same as
> the precision of arg1 in this context, so the same reasoning as above
> applies: once we've established the TYPE_PRECISION condition, we already
> know that an unsigned arg1 constant will fit in an unsigned HWI and
> that a signed arg1 constant will fit in a HWI.  So just checking the
> code is enough.

Ah, indeed.

The patch is ok - thanks for the explanations.

Richard.

> Thanks,
> Richard