On Fri, Nov 15, 2013 at 11:48 AM, Richard Sandiford
<rdsandif...@googlemail.com> wrote:
> Richard Biener <richard.guent...@gmail.com> writes:
>> On Thu, Nov 14, 2013 at 10:04 PM, Richard Sandiford
>> <rdsandif...@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
