From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 37143 invoked by alias); 15 Aug 2019 11:24:14 -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 37028 invoked by uid 89); 15 Aug 2019 11:24:05 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-14.8 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_SHORT,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.1 spammy= X-HELO: mail-lj1-f176.google.com Received: from mail-lj1-f176.google.com (HELO mail-lj1-f176.google.com) (209.85.208.176) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 15 Aug 2019 11:24:02 +0000 Received: by mail-lj1-f176.google.com with SMTP id r9so1920861ljg.5 for ; Thu, 15 Aug 2019 04:24:01 -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=J0M2qNxk6G3JzxqxGjj1eWFga7BuGbADNBKIHxxRUXg=; b=X+xv3SeRHaKKSiNIBYjJO3eCUoCgvLssQVCcfxXkEGSM5a3PaG/MFu6itsNkiLCNQa 4dGtz5o2LBYzQQQ+tmcodjypP1lugX4OQ792E2JTeIXNDDIl8GN68jwkvBBVjZZCgGN1 UWMG+FCeo9e1rbSIDJM9dwbeQ1r94YmFr7i+Z2+4bx3BPYeFdzBDcrf08KXwnrXp8HY8 DYNAYeTpSWhMGJ1U/a51X3SdNpFzSrAJ4o3OveT5ESNc/nDCJoYQx022foBbz3UhHlrp 28k2zwDF2+hENmwDbuH4Ip5pvxFKI3VZyIDJdoTlyESF515qy8o5Vl3DOG7ksP/1aSmH I45w== MIME-Version: 1.0 References: <7719cab1-5e3a-be7a-0260-d89add495c3e@redhat.com> <9586e8af-4552-9535-5bcd-a754fa1d7ccd@redhat.com> <7357e66e-3ca6-dae1-5d22-067f5975677f@redhat.com> In-Reply-To: From: Richard Biener Date: Thu, 15 Aug 2019 11:28:00 -0000 Message-ID: Subject: Re: types for VR_VARYING To: Aldy Hernandez Cc: Jeff Law , gcc-patches , Andrew MacLeod Content-Type: text/plain; charset="UTF-8" X-IsSubscribed: yes X-SW-Source: 2019-08/txt/msg01086.txt.bz2 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?! Sorry, couldn't resist to chime in when seeing the NOP_EXPR build. Last time. Promised. I'm resisting to comment on the supports_type_p abdomination. Richard. > Aldy