From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 54214 invoked by alias); 4 Sep 2018 14:21:16 -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 54205 invoked by uid 89); 4 Sep 2018 14:21:15 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-2.8 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.2 spammy= X-HELO: mail-lj1-f182.google.com Received: from mail-lj1-f182.google.com (HELO mail-lj1-f182.google.com) (209.85.208.182) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 04 Sep 2018 14:21:13 +0000 Received: by mail-lj1-f182.google.com with SMTP id v26-v6so3320804ljj.3 for ; Tue, 04 Sep 2018 07:21:13 -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=6bq0Owb1EZuqOlKGyPjVJqsDf2OMJkuNiUmgFFmV9qQ=; b=tFO3tpTNa+AMpJ9kU2u04ELE4wwldLqQCAyhIqg4/m5yYkyneVXOH7jZ6T98dJfLmp ELlOm4Q35Ovz7y/Be0vnG0+yrtvODU+JBTSa/hwHO2WPslW7cADC8sLYL1xQL7EvFf+N BmzuypxjwkxUaRY2UYGoaaXoh+6HKz+7VTPNNrivrt6hOBASovgOgArNoMpaQB+iLBKb /vQTCCxt62IRGrkHrw9nIZm4yhJtoUSndARKIkpemqkW5Q2MFEocQYls4D3gHMI2nvfT N5l/fFQ6tPXPCL7dyAf8oPNl9wcboCmRLdgc/Ldc2O4xljg6QvxIRlZs7+osBw29Ei2t Fteg== MIME-Version: 1.0 References: <30744ab3-510b-fc67-ad59-906fd54c50cf@redhat.com> In-Reply-To: From: Richard Biener Date: Tue, 04 Sep 2018 14:21: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/msg00192.txt.bz2 On Tue, Sep 4, 2018 at 4:12 PM Aldy Hernandez wrote: > > > > On 09/04/2018 08:49 AM, Richard Biener wrote: > > 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. > > Ah, so it's wide_int_to_tree that wants to see the widest precision. I see. I honestly don't remember exactly but I suppose precision mismatched somehow. Your code was OK with not using widest_ints. > > > >> 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. > > Correct, but in that case we will realize that in wide_int_range_convert > and refuse to do the conversion: > > /* If the conversion is not truncating we can convert the min and > max values and canonicalize the resulting range. Otherwise we > can do the conversion if the size of the range is less than what > the precision of the target type can represent. */ > if (outer_prec >= inner_prec > || wi::rshift (wi::sub (vr0_max, vr0_min), > wi::uhwi (outer_prec, inner_prec), > inner_sign) == 0) so you say the check is also sufficient for all anti-ranges? I see the current VRP code does canonicalization to ranges before running into the CONVERT_EXPR_CODE case. > > > >>> > >>> + 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? > > Well in the ssa-range branch we treat [-MIN,+MAX] as varying. We call > it range_for_type(). Sure. But out_min/max is uninitialized when the functions return false? > Also, the above if(!wide_int_range*) chaining is not necessary in the > ssa-range work. See op_wi() in range-op.c. We just return false and > the caller loop takes care of things. I see. I'm still trying to view the wide-int-range.{cc,h} stuff as a generally useful API rather than just factored out "stuff" to be used by VRP and range-op.c ... Anyhow, let's go with your original patch. I hope I can spend some time going over wide-int-range.{cc,h} and sanitizing its interface (yeah, didn't forget the class thing ... maybe tomorrow before I leave ;)) Richard. > (Andrew's forte is not readable function names ;-). When I have my way > with range-op.c, this op_RANDOM_TWO_LETTERS nonsense will go away.) > > OK for trunk? > Aldy