From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 2888 invoked by alias); 19 Dec 2017 04:52:27 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 2874 invoked by uid 89); 19 Dec 2017 04:52:25 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-11.1 required=5.0 tests=BAYES_00,GIT_PATCH_2,GIT_PATCH_3,KAM_ASCII_DIVIDERS,SPF_HELO_PASS,T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 spammy=ACK, ack X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 19 Dec 2017 04:52:23 +0000 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 9BDC9C059721; Tue, 19 Dec 2017 04:52:22 +0000 (UTC) Received: from localhost.localdomain (ovpn-112-2.rdu2.redhat.com [10.10.112.2]) by smtp.corp.redhat.com (Postfix) with ESMTP id B2A7269284; Tue, 19 Dec 2017 04:52:21 +0000 (UTC) Subject: Re: [005/nnn] poly_int: rtx constants To: gcc-patches@gcc.gnu.org, richard.sandiford@linaro.org References: <871sltvm7r.fsf@linaro.org> <87d15du7dh.fsf@linaro.org> <871sjwpyti.fsf@linaro.org> From: Jeff Law Message-ID: Date: Tue, 19 Dec 2017 04:52:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 MIME-Version: 1.0 In-Reply-To: <871sjwpyti.fsf@linaro.org> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2017-12/txt/msg01216.txt.bz2 On 12/14/2017 06:25 PM, Richard Sandiford wrote: > Jeff Law 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 >>> Alan Hayward >>> David Sherwood >>> >>> 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 >> 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