From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 45395 invoked by alias); 4 Sep 2018 12:50:08 -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 45291 invoked by uid 89); 4 Sep 2018 12:50:07 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-2.9 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.2 spammy=H*r:sk:x207-v6 X-HELO: mail-lf1-f42.google.com Received: from mail-lf1-f42.google.com (HELO mail-lf1-f42.google.com) (209.85.167.42) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 04 Sep 2018 12:50:05 +0000 Received: by mail-lf1-f42.google.com with SMTP id x207-v6so2856556lff.3 for ; Tue, 04 Sep 2018 05:50:04 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=SgoassyUgdnMKqTvVRT0btET+bthm7LUkCJQOWlUbk8=; b=ZVBStr8WnWvkmhyhq1OFo4xP7wmoWfxaUWetQFCAlQklstP1CRryFRETJW4RPYrzJd Rkvt6soTrDMl19SkxfOChPBw86VJW31xcgQ5tdlrC8qqZfDSRci+Wnpz1bwRN+IRO31e J2O7MQPLCPWQE/DLrBO/BKelAJGUYES5SH9MlgOjEBFpsftflUWIjH9gfAHRdc5LCSsc 05pReHWSThFlIJoSCn3948gYQRL7qQDN2uQQ+EyBF1GCrZEx9a1hP6ssKiNRroLKl4am 3FFVe7WNYdm4Kr0h1D9uIAirwrNtHAl1eRIotkWx9gggADAnk5ztvCayqBOcXvdfR5FO uLdw== MIME-Version: 1.0 References: <30744ab3-510b-fc67-ad59-906fd54c50cf@redhat.com> In-Reply-To: <30744ab3-510b-fc67-ad59-906fd54c50cf@redhat.com> From: Richard Biener Date: Tue, 04 Sep 2018 12:50:00 -0000 Message-ID: Subject: Re: VRP: abstract out wide int CONVERT_EXPR_P code To: Aldy Hernandez Cc: GCC Patches Content-Type: text/plain; charset="UTF-8" X-IsSubscribed: yes X-SW-Source: 2018-09/txt/msg00180.txt.bz2 On Tue, Sep 4, 2018 at 2:41 PM Aldy Hernandez wrote: > > > > On 09/04/2018 07:58 AM, Richard Biener wrote: > > On Mon, Sep 3, 2018 at 1:32 PM Aldy Hernandez wrote: > >> > >> > >> > >> On 08/28/2018 05:27 AM, Richard Biener wrote: > >>> On Mon, Aug 27, 2018 at 2:24 PM Aldy Hernandez wrote: > >>>> > >>>> Howdy! > >>>> > >>>> Phew, I think this is the last abstraction. This handles the unary > >>>> CONVERT_EXPR_P code. > >>>> > >>>> It's the usual story-- normalize the symbolics to [-MIN,+MAX] and handle > >>>> everything generically. > >>>> > >>>> Normalizing the symbolics brought about some nice surprises. We now > >>>> handle a few things we were punting on before, which I've documented in > >>>> the patch, but can remove if so desired. I wrote them mainly for myself: > >>>> > >>>> /* NOTES: Previously we were returning VARYING for all symbolics, but > >>>> we can do better by treating them as [-MIN, +MAX]. For > >>>> example, converting [SYM, SYM] from INT to LONG UNSIGNED, > >>>> we can return: ~[0x8000000, 0xffffffff7fffffff]. > >>>> > >>>> We were also failing to convert ~[0,0] from char* to unsigned, > >>>> instead choosing to return VR_VARYING. Now we return ~[0,0]. */ > >>>> > >>>> Tested on x86-64 by the usual bootstrap and regtest gymnastics, > >>>> including --enable-languages=all, because my past sins are still > >>>> haunting me. > >>>> > >>>> OK? > >>> > >>> The new wide_int_range_convert_tree looks odd given it returns > >>> tree's. I'd have expected an API that does the conversion resulting > >>> in a wide_int range and the VRP code adapting to that by converting > >>> the result to trees. > >> > >> Rewritten as suggested. > >> > >> A few notes. > >> > >> First. I am not using widest_int as was done previously. We were > >> passing 0/false to the overflow args in force_fit_type so the call was > >> just degrading into wide_int_to_tree() anyhow. Am I missing some > >> subtlety here, and must we use widest_int and then force_fit_type on the > >> caller? > > > > The issue is that the wide-ints get "converted", so it's indeed subtle. > > This seems like it should be documented-- at the very least in > wide_int_range_convert. I don't think you need it there. > What do you mean "converted"? I'd like to understand this better before > I write out the comment :). Do we lose precision by keeping it in a > wide_int as opposed to a widest_int? As you're using wide_int::from that "changes" precision now. > Also, can we have the caller to wide_int_range_convert() use > wide_int_to_tree directly instead of passing through force_fit_type? I think so. > It > seems force_fit_type with overflowable=0 and overflowed=false is just a > call to wide_int_to_tree anyhow. > > > > > + || wi::rshift (wi::sub (vr0_max, vr0_min), > > + wi::uhwi (outer_prec, inner_prec), > > + inner_sign) == 0) > > > > this was previously allowed only for VR_RANGEs - now it's also for > > VR_ANTI_RANGE. > > That looks incorrect. Testcase welcome ;) > > See change to extract_range_into_wide_ints. VR_ANTI_RANGEs of constants > will remain as is, whereas VR_ANTI_RANGEs of symbols will degrade into > [-MIN,+MAX] and be handled generically. > > Also, see comment: > > + /* We normalize everything to a VR_RANGE, but for constant > + anti-ranges we must handle them by leaving the final result > + as an anti range. This allows us to convert things like > + ~[0,5] seamlessly. */ Yes, but for truncation of ~[0, 5] from say int to short it's not correct to make the result ~[0, 5], is it? At least the original code dropped that to VARYING. For the same reason truncating [3, 765] from short to unsigned char isn't [3, 253]. But maybe I'm missing something. > > > > + return (!wi::eq_p (min, wi::min_value (outer_prec, outer_sign)) > > + || !wi::eq_p (max, wi::max_value (outer_prec, outer_sign))); > > > > so you return false for VARYING? Why? I'd simply return true and > > return VARYING in the new type in the path you return false. > > Since symbols and VR_VARYING and other things get turned into a > [-MIN,+MAX] by extract_range_into_wide_ints, it's a lot easier to handle > things generically and return varying when the range spans the entire > domain. It also keeps with the rest of the wide_int_range_* functions > that return false when we know it's going to be VR_VARYING. Ah, OK, didn't know they did that. Not sure if that's convenient though given VR_VARYING has no representation in the wide-int-range "range" so callers chaining ops would need to do sth like if (!wide_int_range_* (...., &out_min, &out_max)) { out_min = wide_int::min (prec); out_max = wide_int::max (prec); } if (!wide_int_range_* (out_min, out_max, ....)) ... to not lose cases where VARYING inputs produce non-VARYING results? Richard. > Aldy > > > > > Richard. > > > > > > > >> > >> Second. I need extract_range_into_wide_ints to work with anti ranges of > >> constants. It seems like only the unary handling of CONVERT_EXPR here > >> uses it that way, so I believe it should go into one patch. If you > >> believe strongly otherwise, I could split out the 4 lines into a > >> separate patch, but I'd prefer not to. > >> > >> Finally, I could use vr0_min.get_precision() instead of inner_precision, > >> but I think it's more regular and readable the way I have it. > >> > >> Tested on all languages on x86-64 Linux. > >> > >> OK for trunk?