From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by sourceware.org (Postfix) with ESMTP id 3AE3F3857828 for ; Tue, 15 Sep 2020 19:59:43 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 3AE3F3857828 Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-581-5O2dBWupNimT0UYFUL1FTg-1; Tue, 15 Sep 2020 15:59:38 -0400 X-MC-Unique: 5O2dBWupNimT0UYFUL1FTg-1 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 09C18AD682 for ; Tue, 15 Sep 2020 19:59:38 +0000 (UTC) Received: from [10.10.113.83] (ovpn-113-83.rdu2.redhat.com [10.10.113.83]) by smtp.corp.redhat.com (Postfix) with ESMTP id 590CA75141; Tue, 15 Sep 2020 19:59:37 +0000 (UTC) Subject: Re: [PATCH] Allow copying of symbolic ranges to an irange. To: Aldy Hernandez , gcc-patches References: <7451c973-0a40-f8b9-3546-aedd36f63ee2@redhat.com> From: Andrew MacLeod Message-ID: Date: Tue, 15 Sep 2020 15:59:36 -0400 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: <7451c973-0a40-f8b9-3546-aedd36f63ee2@redhat.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 X-Mimecast-Spam-Score: 0.003 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Content-Language: en-US X-Spam-Status: No, score=-12.2 required=5.0 tests=BAYES_00, BODY_8BITS, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H5, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 15 Sep 2020 19:59:44 -0000 On 9/15/20 11:57 AM, Aldy Hernandez wrote: > This fixes an ICE when trying to copy a legacy value_range containing > a symbolic to a multi-range: > >     min = make_ssa_name (type); >     max = build_int_cst (type, 55); >     value_range vv (min, max); >     int_range<2> vr = vv; > > This doesn't affect anything currently, as we don't have a lot of > interactions between value_range's and multi_range's in trunk right, > but it will become a problem as soon as someone tries to get a range > from evrp and copy it over to a multi-range. > > OK pending tests? > > gcc/ChangeLog: > >     * range-op.cc (multi_precision_range_tests): Normalize symbolics > when copying to a multi-range. >     * value-range.cc (irange::copy_legacy_range): Add test. > --- >  gcc/range-op.cc    |  9 +++++++++ >  gcc/value-range.cc | 12 +++++++++++- >  2 files changed, 20 insertions(+), 1 deletion(-) > > diff --git a/gcc/range-op.cc b/gcc/range-op.cc > index c5f511422f4..8e52d5318e9 100644 > --- a/gcc/range-op.cc > +++ b/gcc/range-op.cc > @@ -3463,6 +3463,15 @@ multi_precision_range_tests () >    small = big; >    ASSERT_TRUE (small == int_range<1> (INT (21), INT (21), > VR_ANTI_RANGE)); > > +  // Copying a legacy symbolic to an int_range should normalize the > +  // symbolic at copy time. > +  { > +    value_range legacy_range (make_ssa_name (integer_type_node), INT > (25)); > +    int_range<2> copy = legacy_range; > +    ASSERT_TRUE (copy == int_range<2>  (vrp_val_min (integer_type_node), > +                    INT (25))); > +  } > + >    range3_tests (); >  } > > diff --git a/gcc/value-range.cc b/gcc/value-range.cc > index 20aa4f114c9..26ccd143e5c 100644 > --- a/gcc/value-range.cc > +++ b/gcc/value-range.cc > @@ -101,7 +101,17 @@ irange::copy_legacy_range (const irange &src) >         VR_ANTI_RANGE); >      } >    else > -    set (src.min (), src.max (), VR_RANGE); > +    { > +      // If copying legacy to int_range, normalize any symbolics. > +      if (src.legacy_mode_p () && !range_has_numeric_bounds_p (&src)) > +    { > +      value_range cst (src); > +      cst.normalize_symbolics (); > +      set (cst.min (), cst.max ()); > +      return; > +    } > +      set (src.min (), src.max ()); > +    } >  } > >  // Swap min/max if they are out of order.  Return TRUE if further these seems OK, but can't there be anti-ranges with symbolics  too? ie  ~[a_12, a_12] The code for that just does:  else if (src.kind () == VR_ANTI_RANGE)     set (src.min (), src.max (), VR_ANTI_RANGE); That should just go to varying I guess? The conversion to legacy anti-range code also seems a little suspect in some cases... Finally, we theoretically shouldn't be accessing 'min()' and 'max()' fields in a multirange, which also looks like might happen in the final else clause. I wonder if it might be less complex to simply have 2 routines, like copy_to_legacy()  and copy_from_legacy()  instead of trying to handle then together?  I do find it seems to require more thinking than it should to follow the cases :-) Andrew