Re: [005/nnn] poly_int: rtx constants

[Jeff Law <law@redhat.com>]

On 12/14/2017 06:25 PM, Richard Sandiford wrote:
> Jeff Law <law@redhat.com> writes:
>> On 10/23/2017 11:00 AM, Richard Sandiford wrote:
>>> This patch adds an rtl representation of poly_int values.
>>> There were three possible ways of doing this:
>>>
>>> (1) Add a new rtl code for the poly_ints themselves and store the
>>>     coefficients as trailing wide_ints.  This would give constants like:
>>>
>>>       (const_poly_int [c0 c1 ... cn])
>>>
>>>     The runtime value would be:
>>>
>>>       c0 + c1 * x1 + ... + cn * xn
>>>
>>> (2) Like (1), but use rtxes for the coefficients.  This would give
>>>     constants like:
>>>
>>>       (const_poly_int [(const_int c0)
>>>                        (const_int c1)
>>>                        ...
>>>                        (const_int cn)])
>>>
>>>     although the coefficients could be const_wide_ints instead
>>>     of const_ints where appropriate.
>>>
>>> (3) Add a new rtl code for the polynomial indeterminates,
>>>     then use them in const wrappers.  A constant like c0 + c1 * x1
>>>     would then look like:
>>>
>>>       (const:M (plus:M (mult:M (const_param:M x1)
>>>                                (const_int c1))
>>>                        (const_int c0)))
>>>
>>> There didn't seem to be that much to choose between them.  The main
>>> advantage of (1) is that it's a more efficient representation and
>>> that we can refer to the cofficients directly as wide_int_storage.
>> Well, and #1 feels more like how we handle CONST_INT :-)
>>>
>>>
>>> 2017-10-23  Richard Sandiford  <richard.sandiford@linaro.org>
>>> 	    Alan Hayward  <alan.hayward@arm.com>
>>> 	    David Sherwood  <david.sherwood@arm.com>
>>>
>>> gcc/
>>> 	* doc/rtl.texi (const_poly_int): Document.
>>> 	* gengenrtl.c (excluded_rtx): Return true for CONST_POLY_INT.
>>> 	* rtl.h (const_poly_int_def): New struct.
>>> 	(rtx_def::u): Add a cpi field.
>>> 	(CASE_CONST_UNIQUE, CASE_CONST_ANY): Add CONST_POLY_INT.
>>> 	(CONST_POLY_INT_P, CONST_POLY_INT_COEFFS): New macros.
>>> 	(wi::rtx_to_poly_wide_ref): New typedef
>>> 	(const_poly_int_value, wi::to_poly_wide, rtx_to_poly_int64)
>>> 	(poly_int_rtx_p): New functions.
>>> 	(trunc_int_for_mode): Declare a poly_int64 version.
>>> 	(plus_constant): Take a poly_int64 instead of a HOST_WIDE_INT.
>>> 	(immed_wide_int_const): Take a poly_wide_int_ref rather than
>>> 	a wide_int_ref.
>>> 	(strip_offset): Declare.
>>> 	(strip_offset_and_add): New function.
>>> 	* rtl.def (CONST_POLY_INT): New rtx code.
>>> 	* rtl.c (rtx_size): Handle CONST_POLY_INT.
>>> 	(shared_const_p): Use poly_int_rtx_p.
>>> 	* emit-rtl.h (gen_int_mode): Take a poly_int64 instead of a
>>> 	HOST_WIDE_INT.
>>> 	(gen_int_shift_amount): Likewise.
>>> 	* emit-rtl.c (const_poly_int_hasher): New class.
>>> 	(const_poly_int_htab): New variable.
>>> 	(init_emit_once): Initialize it when NUM_POLY_INT_COEFFS > 1.
>>> 	(const_poly_int_hasher::hash): New function.
>>> 	(const_poly_int_hasher::equal): Likewise.
>>> 	(gen_int_mode): Take a poly_int64 instead of a HOST_WIDE_INT.
>>> 	(immed_wide_int_const): Rename to...
>>> 	(immed_wide_int_const_1): ...this and make static.
>>> 	(immed_wide_int_const): New function, taking a poly_wide_int_ref
>>> 	instead of a wide_int_ref.
>>> 	(gen_int_shift_amount): Take a poly_int64 instead of a HOST_WIDE_INT.
>>> 	(gen_lowpart_common): Handle CONST_POLY_INT.
>>> 	* cse.c (hash_rtx_cb, equiv_constant): Likewise.
>>> 	* cselib.c (cselib_hash_rtx): Likewise.
>>> 	* dwarf2out.c (const_ok_for_output_1): Likewise.
>>> 	* expr.c (convert_modes): Likewise.
>>> 	* print-rtl.c (rtx_writer::print_rtx, print_value): Likewise.
>>> 	* rtlhash.c (add_rtx): Likewise.
>>> 	* explow.c (trunc_int_for_mode): Add a poly_int64 version.
>>> 	(plus_constant): Take a poly_int64 instead of a HOST_WIDE_INT.
>>> 	Handle existing CONST_POLY_INT rtxes.
>>> 	* expmed.h (expand_shift): Take a poly_int64 instead of a
>>> 	HOST_WIDE_INT.
>>> 	* expmed.c (expand_shift): Likewise.
>>> 	* rtlanal.c (strip_offset): New function.
>>> 	(commutative_operand_precedence): Give CONST_POLY_INT the same
>>> 	precedence as CONST_DOUBLE and put CONST_WIDE_INT between that
>>> 	and CONST_INT.
>>> 	* rtl-tests.c (const_poly_int_tests): New struct.
>>> 	(rtl_tests_c_tests): Use it.
>>> 	* simplify-rtx.c (simplify_const_unary_operation): Handle
>>> 	CONST_POLY_INT.
>>> 	(simplify_const_binary_operation): Likewise.
>>> 	(simplify_binary_operation_1): Fold additions of symbolic constants
>>> 	and CONST_POLY_INTs.
>>> 	(simplify_subreg): Handle extensions and truncations of
>>> 	CONST_POLY_INTs.
>>> 	(simplify_const_poly_int_tests): New struct.
>>> 	(simplify_rtx_c_tests): Use it.
>>> 	* wide-int.h (storage_ref): Add default constructor.
>>> 	(wide_int_ref_storage): Likewise.
>>> 	(trailing_wide_ints): Use GTY((user)).
>>> 	(trailing_wide_ints::operator[]): Add a const version.
>>> 	(trailing_wide_ints::get_precision): New function.
>>> 	(trailing_wide_ints::extra_size): Likewise.
>> Do we need to define anything WRT structure sharing in rtl.texi for a
>> CONST_POLY_INT?
> 
> Good catch.  Fixed in the patch below.
> 
>>> Index: gcc/rtl.c
>>> ===================================================================
>>> --- gcc/rtl.c	2017-10-23 16:52:20.579835373 +0100
>>> +++ gcc/rtl.c	2017-10-23 17:00:54.443002147 +0100
>>> @@ -257,9 +261,10 @@ shared_const_p (const_rtx orig)
>>>  
>>>    /* CONST can be shared if it contains a SYMBOL_REF.  If it contains
>>>       a LABEL_REF, it isn't sharable.  */
>>> +  poly_int64 offset;
>>>    return (GET_CODE (XEXP (orig, 0)) == PLUS
>>>  	  && GET_CODE (XEXP (XEXP (orig, 0), 0)) == SYMBOL_REF
>>> -	  && CONST_INT_P (XEXP (XEXP (orig, 0), 1)));
>>> +	  && poly_int_rtx_p (XEXP (XEXP (orig, 0), 1), &offset));
>> Did this just change structure sharing for CONST_WIDE_INT?
> 
> No, we'd only use CONST_WIDE_INT for things that don't fit in
> poly_int64.
> 
>>> +  /* Create a new rtx.  There's a choice to be made here between installing
>>> +     the actual mode of the rtx or leaving it as VOIDmode (for consistency
>>> +     with CONST_INT).  In practice the handling of the codes is different
>>> +     enough that we get no benefit from using VOIDmode, and various places
>>> +     assume that VOIDmode implies CONST_INT.  Using the real mode seems like
>>> +     the right long-term direction anyway.  */
>> Certainly my preference is to get the mode in there.  I see modeless
>> CONST_INTs as a long standing wart and I'm not keen to repeat it.
> 
> Yeah.  Still regularly hit problems related to modeless CONST_INTs
> today (including the gen_int_shift_amount patch).
> 
>>> Index: gcc/wide-int.h
>>> ===================================================================
>>> --- gcc/wide-int.h	2017-10-23 17:00:20.923835582 +0100
>>> +++ gcc/wide-int.h	2017-10-23 17:00:54.445999420 +0100
>>> @@ -613,6 +613,7 @@ #define SHIFT_FUNCTION \
>>>       access.  */
>>>    struct storage_ref
>>>    {
>>> +    storage_ref () {}
>>>      storage_ref (const HOST_WIDE_INT *, unsigned int, unsigned int);
>>>  
>>>      const HOST_WIDE_INT *val;
>>> @@ -944,6 +945,8 @@ struct wide_int_ref_storage : public wi:
>>>    HOST_WIDE_INT scratch[2];
>>>  
>>>  public:
>>> +  wide_int_ref_storage () {}
>>> +
>>>    wide_int_ref_storage (const wi::storage_ref &);
>>>  
>>>    template <typename T>
>> So doesn't this play into the whole question about initialization of
>> these objects.  So I'll defer on this hunk until we settle that
>> question, but the rest is OK.
> 
> Any more thoughts on this?  In the end the 001 patch went in with
> the empty constructors.  Like I say, I'm happy to switch to C++-11
> "= default;" once we require C++11, but I think having well-defined
> implicit construction would make switching to "= default" harder
> in future.
I think we're good to go.  I would have slightly preferred to avoid the
empty ctor, but not enough to raise an objection to Richi's ACK and
ultimately make the switch to = default harder later.

And just to be clear, I'd like to propose we step forward to C++11 in
the gcc-9 timeframe.  I haven't run that by anyone, but that's the
timeframe I'd personally prefer.


jeff