From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 61629 invoked by alias); 7 Dec 2017 22:39:04 -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 61619 invoked by uid 89); 7 Dec 2017 22:39:03 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.7 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.2 spammy= X-HELO: mail-wm0-f49.google.com Received: from mail-wm0-f49.google.com (HELO mail-wm0-f49.google.com) (74.125.82.49) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 07 Dec 2017 22:39:01 +0000 Received: by mail-wm0-f49.google.com with SMTP id 64so470973wme.3 for ; Thu, 07 Dec 2017 14:39:00 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:mail-followup-to:cc:subject:references :date:in-reply-to:message-id:user-agent:mime-version; bh=Hr1na61IG9FYashKGJRI93Sr4vL9b1tJuFTVH/meJOg=; b=NXV0SV3K1Iwt81bi96AbYifJg8AeCLPGZLVDBZbqBqPr9daT8kWG15DyCHJy7nvNRj BAVULh7nLZ8PeEkuU0BK7hiYh+Dn8FVCOM/uoNsBNJUmaWdGnQvzWZlTK1hBmWuZiCHY fO9FvOuKKbO9mdD8KEd4whPOmvKRgK/8Nt1uzaq/2Rq7wcmNOUeR8rA94QJTJcJZaSS6 owXLtyHAki7xQbL55mBPtaz/oiaRCccln8AlPZgJ3KY70v1Z02SXupiUBd0e1vb9wqPp fQBPkvux2C8jKggkxO/9EnUeTBzMjhp9/ZBRpQjqpQg7uUTnaFKlhRdpqVHrerBccvhx Cluw== X-Gm-Message-State: AKGB3mKrq9/Qci4BPaP/+IQvS+dugrX0iBQR9ojcQiQI2K12nbiFlphD 0yPWbPto996zVyQztjcyes6PbMUiI2s= X-Google-Smtp-Source: AGs4zMaaaZV6UdocZrvFDwk6ua0/2GbC6rPyVK1uVnmSjUfaQ6LuXzl1BwsyIZ6fN7P79lcE0Fbp6A== X-Received: by 10.28.1.14 with SMTP id 14mr2529472wmb.51.1512686338794; Thu, 07 Dec 2017 14:38:58 -0800 (PST) Received: from localhost ([2.25.234.120]) by smtp.gmail.com with ESMTPSA id j132sm1763852wmd.2.2017.12.07.14.38.57 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Thu, 07 Dec 2017 14:38:57 -0800 (PST) From: Richard Sandiford To: Jeff Law Mail-Followup-To: Jeff Law ,Richard Biener , GCC Patches , richard.sandiford@linaro.org Cc: Richard Biener , GCC Patches Subject: Re: [001/nnn] poly_int: add poly-int.h References: <871sltvm7r.fsf@linaro.org> <87vaj5u7id.fsf@linaro.org> <87efp95c9b.fsf@linaro.org> <87375hvi77.fsf@linaro.org> <4a0e05b6-e5ee-a21d-8240-02bb5e246ee2@redhat.com> Date: Thu, 07 Dec 2017 22:39:00 -0000 In-Reply-To: <4a0e05b6-e5ee-a21d-8240-02bb5e246ee2@redhat.com> (Jeff Law's message of "Thu, 7 Dec 2017 08:08:19 -0700") Message-ID: <87efo6f9j3.fsf@linaro.org> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-SW-Source: 2017-12/txt/msg00424.txt.bz2 Jeff Law writes: > On 12/07/2017 07:46 AM, Richard Biener wrote: >> On Wed, Dec 6, 2017 at 9:11 PM, Jeff Law wrote: >>> On 11/13/2017 05:04 PM, Richard Sandiford wrote: >>>> Richard Sandiford writes: >>>>> Richard Sandiford writes: >>>>>> 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. >>>>> >>>>> Changes in v2: >>>>> >>>>> - Drop the controversial known_zero etc. wrapper functions. >>>>> - Fix the operator<<= bug that Martin found. >>>>> - Switch from "t" to "type" in SFINAE classes (requested by Martin). >>>>> >>>>> Not changed in v2: >>>>> >>>>> - Default constructors are still empty. I agree it makes sense to use >>>>> "= default" when we switch to C++11, but it would be dangerous for >>>>> that to make "poly_int64 x;" less defined than it is now. >>>> >>>> After talking about this a bit more internally, it was obvious that >>>> the choice of "must" and "may" for the predicate names was a common >>>> sticking point. The idea was to match the names of alias predicates, >>>> but given my track record with names ("too_empty_p" being a recently >>>> questioned example :-)), I'd be happy to rename them to something else. >>>> Some alternatives we came up with were: >>> I didn't find the must vs may naming problematical as I was going >>> through the changes. What I did find much more difficult was >>> determining if the behavior was correct when we used a "may" predicate. >>> It really relies a good deal on knowing the surrounding code. >>> >>> In places where I knew the code reasonably well could tell without much >>> surrounding context. In other places I had to look at the code and >>> deduce proper behavior in the "may" cases -- and often I resorted to >>> spot checking and relying on your reputation & testing to DTRT. >>> >>> >>>> >>>> - known_eq / maybe_eq / known_lt / maybe_lt etc. >>>> >>>> Some functions already use "known" and "maybe", so this would arguably >>>> be more consistent than using "must" and "may". >>>> >>>> - always_eq / sometimes_eq / always_lt / sometimes_lt >>>> >>>> Similar to the previous one in intent. It's just a question of which >>>> wordng is clearer. >>>> >>>> - forall_eq / exists_eq / forall_lt / exists_lt etc. >>>> >>>> Matches the usual logic quantifiers. This seems quite appealing, >>>> as long as it's obvious that in: >>>> >>>> forall_eq (v0, v1) >>>> >>>> v0 and v1 themselves are already bound: if vi == ai + bi*X then >>>> what we really saying is: >>>> >>>> forall X, a0 + b0*X == a1 + b1*X >>>> >>>> Which of those sounds best? Any other suggestions? >>> I can live with any of them. I tend to prefer one of the first two, but >>> it's not a major concern for me. So if you or others have a clear >>> preference, go with it. >> >> Whatever you do use a consistent naming which I guess means >> using known_eq / maybe_eq? >> >> Otherwise ok. > So I think that's the final ack on this series. Thanks to both of you, really appreciate it! > Richard S. can you confirm? I fully expect the trunk has moved some > and the patches will need adjustments -- consider adjustments which > work in a manner similar to the patches to date pre-approved. Yeah, that's now all of the poly_int patches. I still owe you replies to some of them -- I'll get to that as soon as I can. I'll make the name changes and propagate through the series and then commit this first patch. I was thinking that for the rest it would make sense to commit them individually, with individual testing of each patch, so that it's easier to bisect. I'll try to make sure I don't repeat the merge mistake in the machine-mode series. I think it'd also make sense to divide the commits up into groups rather than do them all at once, since it's easier to do the individual testing that way. Does that sound OK? Thanks, Richard