From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 4120 invoked by alias); 4 Oct 2019 16:02:06 -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 4112 invoked by uid 89); 4 Oct 2019 16:02:06 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-14.6 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.1 spammy=seriously 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; Fri, 04 Oct 2019 16:02:04 +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 53C11300D20F for ; Fri, 4 Oct 2019 16:02:03 +0000 (UTC) Received: from localhost.localdomain (ovpn-112-68.rdu2.redhat.com [10.10.112.68]) by smtp.corp.redhat.com (Postfix) with ESMTP id 568FD5C1D8; Fri, 4 Oct 2019 16:02:02 +0000 (UTC) Subject: Re: [patch] canonicalize unsigned [1,MAX] ranges into ~[0,0] To: Aldy Hernandez Cc: gcc-patches , Andrew MacLeod References: <995b4560-6a76-6742-888f-eadbfb9ff9cc@redhat.com> <74fb2cf8-6312-6ae2-b620-3d36deb61080@redhat.com> From: Jeff Law Openpgp: preference=signencrypt Message-ID: <6a557c4c-d86b-7b7f-70c2-483fff2d5f59@redhat.com> Date: Fri, 04 Oct 2019 16:02:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.4.0 MIME-Version: 1.0 In-Reply-To: <74fb2cf8-6312-6ae2-b620-3d36deb61080@redhat.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-IsSubscribed: yes X-SW-Source: 2019-10/txt/msg00349.txt.bz2 On 10/4/19 9:49 AM, Aldy Hernandez wrote: > > > On 10/4/19 11:38 AM, Jeff Law wrote: >> On 10/4/19 6:59 AM, Aldy Hernandez wrote: >>> When I did the value_range canonicalization work, I noticed that an >>> unsigned [1,MAX] and an ~[0,0] could be two different representations >>> for the same thing.  I didn't address the problem then because callers >>> to ranges_from_anti_range() would go into an infinite loop trying to >>> extract ~[0,0] into [1,MAX] and [].  We had a lot of callers to >>> ranges_from_anti_range, and it smelled like a rat's nest, so I bailed. >>> >>> Now that we have one main caller (from the symbolic PLUS/MINUS >>> handling), it's a lot easier to contain.  Well, singleton_p also calls >>> it, but it's already handling nonzero specially, so it wouldn't be affected. >>> >>> >>> >>> With some upcoming cleanups I'm about to post, the fact that [1,MAX] and >>> ~[0,0] are equal_p(), but not nonzero_p(), matters.  Plus, it's just >>> good form to have one representation, giving us the ability to pick at >>> nonzero_p ranges with ease. >>> >>> The code in extract_range_from_plus_minus_expr() continues to be a mess >>> (as it has always been), but at least it's contained, and with this >>> patch, it's slightly smaller. >>> >>> Note, I'm avoiding adding a comment header for functions with highly >>> descriptive obvious names. >>> >>> OK? >>> >>> Aldy >>> >>> canonicalize-nonzero-ranges.patch >>> >>> commit 1c333730deeb4ddadc46ad6d12d5344f92c0352c >>> Author: Aldy Hernandez >>> Date:   Fri Oct 4 08:51:25 2019 +0200 >>> >>>      Canonicalize UNSIGNED [1,MAX] into ~[0,0]. >>>           Adapt PLUS/MINUS symbolic handling, so it doesn't call >>>      ranges_from_anti_range with a VR_ANTI_RANGE containing one >>> sub-range. >>> >>> diff --git a/gcc/ChangeLog b/gcc/ChangeLog >>> index 6e4f145af46..3934b41fdf9 100644 >>> --- a/gcc/ChangeLog >>> +++ b/gcc/ChangeLog >>> @@ -1,3 +1,18 @@ >>> +2019-10-04  Aldy Hernandez  >>> + >>> +    * tree-vrp.c (value_range_base::singleton_p): Use num_pairs >>> +    instead of calling vrp_val_is_*. >>> +    (value_range_base::set): Canonicalize unsigned [1,MAX] into >>> +    non-zero. >>> +    (range_has_numeric_bounds_p): New. >>> +    (range_int_cst_p): Use range_has_numeric_bounds_p. >>> +    (ranges_from_anti_range): Assert that we won't recurse >>> +    indefinitely. >>> +    (extract_extremes_from_range): New. >>> +    (extract_range_from_plus_minus_expr): Adapt so we don't call >>> +    ranges_from_anti_range with an anti-range containing only one >>> +    sub-range. >> So no problem with the implementation, but I do have a higher level >> question. >> >> One of the goals of the representation side of the Ranger project is to >> drop anti-ranges.  Canonicalizing [1, MAX] to ~[0,0] seems to be going >> in the opposite direction.   So do we really want to canonicalize to >> ~[0,0]? > > Hmmm, Andrew had the same question. > > It really doesn't matter what we canonicalize too, as long as we're > consistent, but there are a bunch of non-zero tests throughout that were > checking for the ~[0,0] construct, and I didn't want to rock the boat > too much.  Although in all honesty, most of those should already be > converted to the ::nonzero_p() API. > > However, if we canonicalize into [1,MAX] for unsigned, we have the > problem that a signed non-zero will still be ~[0,0], so our ::nonzero_p > code will have to check two different representations, not to mention it > will now have to check TYPE_UNSIGNED(type). ISTM that the right thing to do is to first move to the ::nonzero_p API, which should be a behavior preserving change. It'd probably be a medium sized change, but highly mechanical and behavior preserving, so easy to review. Once that's in place, then I'd seriously look at [1,MAX] as the canonical form for unsigned types and [MIN, -1] [1, MAX] as the canonical form for signed types. If we're trying to get away from ANTI ranges, canonicalizing on ~[0,0] just seems wrong. Where things may get painful is things like [MIN, -3] [3, MAX] which occur due to arithmetic and pointer alignments. I think we have a hack somewhere which special cases this into [MIN, -1], [1, MAX] even though it's technically less precise. jeff