From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 60119 invoked by alias); 16 Aug 2019 06:57:29 -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 60111 invoked by uid 89); 16 Aug 2019 06:57:28 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-21.9 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_SHORT,RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.1 spammy= X-HELO: mail-wm1-f66.google.com Received: from mail-wm1-f66.google.com (HELO mail-wm1-f66.google.com) (209.85.128.66) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 16 Aug 2019 06:57:18 +0000 Received: by mail-wm1-f66.google.com with SMTP id p74so3185036wme.4 for ; Thu, 15 Aug 2019 23:57:18 -0700 (PDT) Return-Path: Received: from abulafia.quesejoda.com (185.red-79-157-157.dynamicip.rima-tde.net. [79.157.157.185]) by smtp.gmail.com with ESMTPSA id t8sm9719703wra.73.2019.08.15.23.57.14 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 15 Aug 2019 23:57:15 -0700 (PDT) Subject: Re: types for VR_VARYING From: Aldy Hernandez To: Richard Biener Cc: Jeff Law , gcc-patches , Andrew MacLeod References: <7719cab1-5e3a-be7a-0260-d89add495c3e@redhat.com> <9586e8af-4552-9535-5bcd-a754fa1d7ccd@redhat.com> <7357e66e-3ca6-dae1-5d22-067f5975677f@redhat.com> Message-ID: Date: Fri, 16 Aug 2019 08:47:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.7.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit X-IsSubscribed: yes X-SW-Source: 2019-08/txt/msg01149.txt.bz2 On 8/15/19 12:06 PM, Aldy Hernandez wrote: > > > On 8/15/19 7:23 AM, Richard Biener wrote: >> On Thu, Aug 15, 2019 at 12:40 PM Aldy Hernandez wrote: >>> >>> On 8/14/19 1:37 PM, Jeff Law wrote: >>>> On 8/13/19 6:39 PM, Aldy Hernandez wrote: >>>>> >>>>> >>>>> On 8/12/19 7:46 PM, Jeff Law wrote: >>>>>> On 8/12/19 12:43 PM, Aldy Hernandez wrote: >>>>>>> This is a fresh re-post of: >>>>>>> >>>>>>> https://gcc.gnu.org/ml/gcc-patches/2019-07/msg00006.html >>>>>>> >>>>>>> Andrew gave me some feedback a week ago, and I obviously don't >>>>>>> remember >>>>>>> what it was because I was about to leave on PTO.  However, I do >>>>>>> remember >>>>>>> I addressed his concerns before getting drunk on rum in tropical >>>>>>> islands. >>>>>>> >>>>>> FWIW found a great coffee infused rum while in Kauai last week. >>>>>> I'm not >>>>>> a coffee fan, but it was wonderful.  The one bottle we brought back >>>>>> isn't going to last until Cauldron and I don't think I can get a >>>>>> special >>>>>> order filled before I leave :( >>>>> >>>>> You must bring some to Cauldron before we believe you. :) >>>> That's the problem.  The nearest place I can get it is in Vegas and >>>> there's no distributor in Montreal.   I can special order it in our >>>> state run stores, but it won't be here in time. >>>> >>>> Of course, I don't mind if you don't believe me.  More for me in that >>>> case... >>>> >>>> >>>>>> Is the supports_type_p stuff there to placate the calls from >>>>>> ipa-cp?  I >>>>>> can live with it in the short term, but it really feels like there >>>>>> should be something in the ipa-cp client that avoids this silliness. >>>>> >>>>> I am not happy with this either, but there are various places where >>>>> statements that are !stmt_interesting_for_vrp() are still setting a >>>>> range of VARYING, which is then being ignored at a later time. >>>>> >>>>> For example, vrp_initialize: >>>>> >>>>>         if (!stmt_interesting_for_vrp (phi)) >>>>>           { >>>>>             tree lhs = PHI_RESULT (phi); >>>>>             set_def_to_varying (lhs); >>>>>             prop_set_simulate_again (phi, false); >>>>>           } >>>>> >>>>> Also in evrp_range_analyzer::record_ranges_from_stmt(), where we if >>>>> the >>>>> statement is interesting for VRP but extract_range_from_stmt() does >>>>> not >>>>> produce a useful range, we also set a varying for a range we will >>>>> never >>>>> use.  Similarly for a statement that is not interesting in this hunk. >>>> Ugh.  One could perhaps argue that setting any kind of range in these >>>> circumstances is silly.   But I suspect it's necessary due to the >>>> optimistic handling of VR_UNDEFINED in value_range_base::union_helper. >>>> It's all coming back to me now... >>>> >>>> >>>>> >>>>> Then there is vrp_prop::visit_stmt() where we also set VARYING for >>>>> types >>>>> that VRP will never handle: >>>>> >>>>>         case IFN_ADD_OVERFLOW: >>>>>         case IFN_SUB_OVERFLOW: >>>>>         case IFN_MUL_OVERFLOW: >>>>>         case IFN_ATOMIC_COMPARE_EXCHANGE: >>>>>       /* These internal calls return _Complex integer type, >>>>>          which VRP does not track, but the immediate uses >>>>>          thereof might be interesting.  */ >>>>>       if (lhs && TREE_CODE (lhs) == SSA_NAME) >>>>>         { >>>>>           imm_use_iterator iter; >>>>>           use_operand_p use_p; >>>>>           enum ssa_prop_result res = SSA_PROP_VARYING; >>>>> >>>>>           set_def_to_varying (lhs); >>>>> >>>>> I've adjusted the patch so that set_def_to_varying will set the >>>>> range to >>>>> VR_UNDEFINED if !supports_type_p.  This is a fail safe, as we can't >>>>> really do anything with a nonsensical range.  I just don't want to >>>>> leave >>>>> the range in an indeterminate state. >>>>> >>>> I think VR_UNDEFINED is unsafe due to value_range_base::union_helper. >>>> And that's a more general than this patch.  VR_UNDEFINED is _not_ a >>>> safe >>>> range to set something to if we can't handle it.  We have to use >>>> VR_VARYING. >>>> >>>> Why?  See the beginning of value_range_base::union_helper: >>>> >>>>      /* VR0 has the resulting range if VR1 is undefined or VR0 is >>>> varying.  */ >>>>      if (vr1->undefined_p () >>>>          || vr0->varying_p ()) >>>>        return *vr0; >>>> >>>>      /* VR1 has the resulting range if VR0 is undefined or VR1 is >>>> varying.  */ >>>>      if (vr0->undefined_p () >>>>          || vr1->varying_p ()) >>>>        return *vr1; >>>> This can get called for something like >>>> >>>>     a = ? name1 : name2; >>>> >>>> If name1 was set to VR_UNDEFINED thinking that VR_UNDEFINED was a safe >>>> value for something we can't handle, then we'll incorrectly return the >>>> range for name2. >>> >>> I think that if name1 was !supports_type_p, we will have never called >>> union/intersect.  We will have bailed at some point earlier.  However, I >>> do see your point about being consistent. >>> >>>> >>>> VR_UNDEFINED can only be used for the ranges of objects we haven't >>>> processed.  If we can't produce a range for an object because the >>>> statement is something we don't handle or just doesn't produce anythign >>>> useful, then the right result is VR_VARYING. >>>> >>>> This may be worth commenting at the definition site for VR_*. >>>> >>>>> >>>>> I also noticed that Andrew's patch was setting num_vr_values to >>>>> num_ssa_names + num_ssa_names / 10.  I think he meant num_vr_values + >>>>> num_vr_values / 10.  Please verify the current incantation makes >>>>> sense. >>>> Going to assume this will be adjusted per the other messages in this >>>> thread. >>> >>> Done. >>> >>>> >>>> >>>>> diff --git a/gcc/tree-ssa-threadedge.c b/gcc/tree-ssa-threadedge.c >>>>> index 39ea22f0554..663dd6e2398 100644 >>>>> --- a/gcc/tree-ssa-threadedge.c >>>>> +++ b/gcc/tree-ssa-threadedge.c >>>>> @@ -182,8 +182,10 @@ record_temporary_equivalences_from_phis (edge e, >>>>>           new_vr->deep_copy (vr_values->get_value_range (src)); >>>>>         else if (TREE_CODE (src) == INTEGER_CST) >>>>>           new_vr->set (src); >>>>> +      else if (value_range_base::supports_type_p (TREE_TYPE (src))) >>>>> +        new_vr->set_varying (TREE_TYPE (src)); >>>>>         else >>>>> -        new_vr->set_varying (); >>>>> +        new_vr->set_undefined (); >>>> So I think this can cause problems.  VR_VARYING seems like the right >>>> state here. >>>> >>>> >>>>> + >>>>> +  if (!value_range_base::supports_type_p (TREE_TYPE (var))) >>>>> +    { >>>>> +      vr->set_undefined (); >>>>> +      return vr; >>>> Probably better as VR_VARYING here too. >>>> >>>>> +    { >>>>> +      /* If we have an unsupported type (structs, void, etc), there >>>>> +         is nothing we'll be able to do with this entry. >>>>> +         Initialize it to UNDEFINED as a sanity measure, just in >>>>> +         case.  */ >>>>> +      vr->set_undefined (); >>>> Here too. >>> >>> Hmmm, the problem with setting VR_VARYING for unsupported types is that >>> we have no min/max to use.  Even though min/max will not be used in any >>> calculation, it's nice to have it set so type() will work consistently. >>> May I suggest this generic approach while we disassociate the lattice >>> and ranges from value_range_base (or remove vrp altogether :))? >>> >>> void >>> value_range_base::set_varying (tree type) >>> { >>>     m_kind = VR_VARYING; >>>     if (supports_type_p (type)) >>>       { >>>         m_min = vrp_val_min (type, true); >>>         m_max = vrp_val_max (type, true); >>>       } >>>     else >>>       { >>>         /* We can't do anything range-wise with these types.  Build >>>           something for which type() will work as a temporary measure >>>           until lattices and value_range_base are disassociated.  */ >>>         m_min = m_max = build1 (NOP_EXPR, type, void_type_node); >>>       } >>> } >>> >>> This way, no changes happen throughout the code, varying remains >>> varying, type() works everywhere, and we don't have to dig into all >>> value_range users to skip unsupported types. >>> >>> This should work, as no one is going to call min() / max() on an >>> unsupported type, since they're just being used for the lattice. >> >> Then use error_mark_node or NULL_TREE?! > > I tested with error_mark_node.  It seems no one is calling type() on > these unsupported types, so perhaps error_mark_node is fine, and signals > that we don't support these types if anyone tries to do anything funny > with them.  I've adjusted the patch. Committed to trunk. Aldy