From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 68197 invoked by alias); 25 Oct 2017 16:14:39 -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 67981 invoked by uid 89); 25 Oct 2017 16:14:39 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-0.5 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,RCVD_IN_DNSWL_NONE,RCVD_IN_SORBS_SPAM,SPF_PASS,URIBL_BLACK autolearn=no version=3.3.2 spammy=impression, poly, flavors X-HELO: mail-oi0-f41.google.com Received: from mail-oi0-f41.google.com (HELO mail-oi0-f41.google.com) (209.85.218.41) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 25 Oct 2017 16:14:37 +0000 Received: by mail-oi0-f41.google.com with SMTP id m198so927177oig.5 for ; Wed, 25 Oct 2017 09:14:36 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-transfer-encoding; bh=Vqzxr9YaDh4jFauakD33aNRURiK4LfHUZ8KJ88K6p6I=; b=cRvo8tpQY5fxid3O45eNqWcTyiFuqNQrseVdUFZUXQRvIbIZsCzDERzCPhSTK+QhPS ZDyOjTThVDaJNm4Zva9K7BdKyqICC8wQLW/xi5KyljHXdm/V1vLrabI+sRsrrW9btlc4 m9/a2x0cMAqdg+x9mtO8vWPBoquwR4d4UZ18HBi6QYWB1U6W7UJcrfqvEbphIrqLXY0O vbxvJv1hKUyfO0ZtJ0i4bri8MOhibZ1baRAL4IVB8u2EKfiDBzcz7Uj/iLze8OQHuTvs axPLnL/CqMQrNNFuH3+B+fm1Vjmcfq/VHoRtPD3fFCDycUkyA3Qd0Rj0K9xHpxPmnNl2 BB1w== X-Gm-Message-State: AMCzsaVxLQqLyLtUGeQ5W7y3SX2a41CvjJzgTeSPybf0VX337+AWJbWS N2vtA7X+oEK6z6ZGdcNgc+A= X-Google-Smtp-Source: ABhQp+T/ggGqgB8psT/3VvxmcRfzd8VhYOUjwct5H6+eSgVVRGmgUG3byL9cpjLNyGkTaZOA5rSU0w== X-Received: by 10.202.245.11 with SMTP id t11mr1333349oih.239.1508948075244; Wed, 25 Oct 2017 09:14:35 -0700 (PDT) Received: from localhost.localdomain (71-218-5-6.hlrn.qwest.net. [71.218.5.6]) by smtp.gmail.com with ESMTPSA id m41sm1576484otd.64.2017.10.25.09.14.34 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 25 Oct 2017 09:14:34 -0700 (PDT) Subject: Re: [001/nnn] poly_int: add poly-int.h To: gcc-patches@gcc.gnu.org, richard.sandiford@linaro.org References: <871sltvm7r.fsf@linaro.org> <87vaj5u7id.fsf@linaro.org> From: Martin Sebor Message-ID: Date: Wed, 25 Oct 2017 16:17:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: <87vaj5u7id.fsf@linaro.org> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2017-10/txt/msg01823.txt.bz2 On 10/23/2017 10:57 AM, Richard Sandiford wrote: > This patch adds a new "poly_int" class to represent polynomial integers > of the form: > > C0 + C1*X1 + C2*X2 ... + Cn*Xn > > It also adds poly_int-based typedefs for offsets and sizes of various > precisions. In these typedefs, the Ci coefficients are compile-time > constants and the Xi indeterminates are run-time invariants. The number > of coefficients is controlled by the target and is initially 1 for all > ports. > > Most routines can handle general coefficient counts, but for now a few > are specific to one or two coefficients. Support for other coefficient > counts can be added when needed. > > The patch also adds a new macro, IN_TARGET_CODE, that can be > set to indicate that a TU contains target-specific rather than > target-independent code. When this macro is set and the number of > coefficients is 1, the poly-int.h classes define a conversion operator > to a constant. This allows most existing target code to work without > modification. The main exceptions are: > > - values passed through ..., which need an explicit conversion to a > constant > > - ?: expression in which one arm ends up being a polynomial and the > other remains a constant. In these cases it would be valid to convert > the constant to a polynomial and the polynomial to a constant, so a > cast is needed to break the ambiguity. > > The patch also adds a new target hook to return the estimated > value of a polynomial for costing purposes. > > The patch also adds operator<< on wide_ints (it was already defined > for offset_int and widest_int). I think this was originally excluded > because >> is ambiguous for wide_int, but << is useful for converting > bytes to bits, etc., so is worth defining on its own. The patch also > adds operator% and operator/ for offset_int and widest_int, since those > types are always signed. These changes allow the poly_int interface to > be more predictable. > > I'd originally tried adding the tests as selftests, but that ended up > bloating cc1 by at least a third. It also took a while to build them > at -O2. The patch therefore uses plugin tests instead, where we can > force the tests to be built at -O0. They still run in negligible time > when built that way. > > > 2017-10-23 Richard Sandiford > Alan Hayward > David Sherwood > > gcc/ > * poly-int.h: New file. > * poly-int-types.h: Likewise. > * coretypes.h: Include them. > (POLY_INT_CONVERSION): Define. > * target.def (estimated_poly_value): New hook. > * doc/tm.texi.in (TARGET_ESTIMATED_POLY_VALUE): New hook. > * doc/tm.texi: Regenerate. > * doc/poly-int.texi: New file. > * doc/gccint.texi: Include it. > * doc/rtl.texi: Describe restrictions on subreg modes. > * Makefile.in (TEXI_GCCINT_FILES): Add poly-int.texi. > * genmodes.c (NUM_POLY_INT_COEFFS): Provide a default definition. > (emit_insn_modes_h): Emit a definition of NUM_POLY_INT_COEFFS. > * targhooks.h (default_estimated_poly_value): Declare. > * targhooks.c (default_estimated_poly_value): New function. > * target.h (estimated_poly_value): Likewise. > * wide-int.h (WI_UNARY_RESULT): Use wi::binary_traits. > (wi::unary_traits): Delete. > (wi::binary_traits::signed_shift_result_type): Define for > offset_int << HOST_WIDE_INT, etc. > (generic_wide_int::operator <<=): Define for all types and use > wi::lshift instead of <<. > (wi::hwi_with_prec): Add a default constructor. > (wi::ints_for): New class. > (operator <<): Define for all wide-int types. > (operator /): New function. > (operator %): Likewise. > * selftest.h (ASSERT_MUST_EQ, ASSERT_MUST_EQ_AT, ASSERT_MAY_NE) > (ASSERT_MAY_NE_AT): New macros. > > gcc/testsuite/ > * gcc.dg/plugin/poly-int-tests.h, > gcc.dg/plugin/poly-int-test-1.c, > gcc.dg/plugin/poly-int-01_plugin.c, > gcc.dg/plugin/poly-int-02_plugin.c, > gcc.dg/plugin/poly-int-03_plugin.c, > gcc.dg/plugin/poly-int-04_plugin.c, > gcc.dg/plugin/poly-int-05_plugin.c, > gcc.dg/plugin/poly-int-06_plugin.c, > gcc.dg/plugin/poly-int-07_plugin.c: New tests. > * gcc.dg/plugin/plugin.exp: Run them. I haven't done nearly a thorough review but the dtor followed by the placement new in the POLY_SET_COEFF() macro caught my eye so I thought I'd ask sooner rather than later. Given the macro definition: + The dummy comparison against a null C * is just a way of checking + that C gives the right type. */ +#define POLY_SET_COEFF(C, RES, I, VALUE) \ + ((void) (&(RES).coeffs[0] == (C *) 0), \ + wi::int_traits::precision_type == wi::FLEXIBLE_PRECISION \ + ? (void) ((RES).coeffs[I] = VALUE) \ + : (void) ((RES).coeffs[I].~C (), new (&(RES).coeffs[I]) C (VALUE))) is the following use well-defined? +template +inline poly_int_pod& +poly_int_pod::operator <<= (unsigned int a) +{ + POLY_SET_COEFF (C, *this, 0, this->coeffs[0] << a); It looks to me as though the VALUE argument in the ctor invoked by the placement new expression is evaluated after the dtor has destroyed the very array element the VALUE argument expands to. Or am I misreading the code? Whether or not is, in fact, a problem, it seems to me that using a function template rather than a macro would be a clearer and safer way to do the same thing. (Safer in that the macro also evaluates its arguments multiple times, which is often a source of subtle bugs.) Other than that, I would suggest changing 't' to something a bit less terse, like perhaps 'type' in traits like the following: +struct if_lossless; +template +struct if_lossless +{ + typedef T3 t; +}; Lastly (for now), I note that the default ply_int ctor, like that of the other xxx_int types, is a no-op. That makes using all these types error prone, e.g., as arrays in ctor-initializer lists: struct Pair { poly_int<...> poly[2]; Pair (): poly () { } // poly[] (unexpectedly) uninitialized } Martin PS My initial interest in this class was to to see if the it is less prone to error than wide_int and offset_int. Specifically, if it's easier to convert the various flavors of xxx_int among one another and between basic integers and the xxx_ints. But after reading the documentation I have the impression it might help with some of the range work I've been doing recently, so I'll try to do a more thorough review in the (hopefully) near future.