Re: Remove host_integerp tests with variable signedness

[Richard Sandiford <rdsandiford at googlemail dot com>]

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.

Thanks,
Richard