From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 50731 invoked by alias); 18 Jul 2016 11:51:18 -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 50714 invoked by uid 89); 18 Jul 2016 11:51:17 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.3 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,RCVD_IN_DNSWL_LOW,SPF_PASS autolearn=ham version=3.3.2 spammy=Discover, churn, recursively, Early X-HELO: mail-wm0-f47.google.com Received: from mail-wm0-f47.google.com (HELO mail-wm0-f47.google.com) (74.125.82.47) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-GCM-SHA256 encrypted) ESMTPS; Mon, 18 Jul 2016 11:51:07 +0000 Received: by mail-wm0-f47.google.com with SMTP id o80so112874418wme.1 for ; Mon, 18 Jul 2016 04:51:06 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=uZ5PiaQSesTaFe5F99bjJwrWS6dEqF93T9tDA07NMow=; b=jw+pS4Wz9Lua3NSMkOIXW3tvheqHEDV5y/n5TQnPaZCLMd5ZXUeyo/5wThJxejJUMd 6S8YQpsO3r2Na6Mdz//FUZByUkDo/eaKzyVYAq/ZlO9X23AbIp5x52QEzdzHu4oJws1u wocN4FR+xfSL+iU+NpGel0Fa/i+lsDPV5Rr9Wbt4U7mglSqezH3VPxnBZ9FdyXwwAgpJ XT3tQy2+82H/jVnhaVSYW9mbGsh3I7l2mqZGc2s9Wtex8Yf5/d742EjjTHyu7u3n/yug 6r8BIiDe93aDUf+gftQUkX+p7huFuNp2cTmFo3KSWFHkrmLIsGALtcD6VxDaC5ctrhJD IMWw== X-Gm-Message-State: ALyK8tIfx09Yst6MNJl+0EGeu4X3PjwpEdyt6fFOOlETGmjNpEXiNIVbDdtaA4a0H60MEYXmMUg8Hnk+kUixlA== X-Received: by 10.28.58.7 with SMTP id h7mr320616wma.35.1468842664253; Mon, 18 Jul 2016 04:51:04 -0700 (PDT) MIME-Version: 1.0 Received: by 10.28.207.135 with HTTP; Mon, 18 Jul 2016 04:51:02 -0700 (PDT) In-Reply-To: <578891C8.7090609@linaro.org> References: <57886949.8010300@linaro.org> <57886A71.6030103@linaro.org> <57888BD6.6070200@linaro.org> <578891C8.7090609@linaro.org> From: Richard Biener Date: Mon, 18 Jul 2016 11:51:00 -0000 Message-ID: Subject: Re: [RFC][IPA-VRP] Early VRP Implementation To: kugan Cc: Andrew Pinski , "gcc-patches@gcc.gnu.org" , Jan Hubicka , Martin Jambor Content-Type: text/plain; charset=UTF-8 X-IsSubscribed: yes X-SW-Source: 2016-07/txt/msg01026.txt.bz2 On Fri, Jul 15, 2016 at 9:33 AM, kugan wrote: > Hi Andrew, > > On 15/07/16 17:28, Andrew Pinski wrote: >> >> On Fri, Jul 15, 2016 at 12:08 AM, kugan >> wrote: >>> >>> Hi Andrew, >>> >>>> Why separate out early VRP from tree-vrp? Just a little curious. >>> >>> >>> >>> It is based on the discussion in >>> https://gcc.gnu.org/ml/gcc/2016-01/msg00069.html. >>> In summary, conclusion (based on my understanding) was to implement a >>> simplified VRP algorithm that doesn't require ASSERT_EXPR insertion. >> >> >> But I don't see why you are moving it from tree-vrp.c . That was my >> question, you pointing to that discussion does not say to split it >> into a new file and expose these interfaces. >> > > Are you saying that I should keep this part of tree-vrp.c. I am happy to do > that if this is considered the best approach. Yes, I think that's the best approach. Can you, as a refactoring before your patch, please change VRP to use an alloc-pool for allocating value_range? The DOM-based VRP will add a lot of malloc/free churn otherwise. Generally watch coding-style such as missed function comments. As you do a non-iterating VRP and thus do not visit back-edges you don't need to initialize loops or SCEV nor do you need loop-closed SSA. As you do a DOM-based VRP using SSA propagator stuff like ssa_prop_result doesn't make any sense. +edge evrp_dom_walker::before_dom_children (basic_block bb) +{ + /* If we are going out of scope, restore the old VR. */ + while (!cond_stack.is_empty () + && !dominated_by_p (CDI_DOMINATORS, bb, cond_stack.last ().first)) + { + tree var = cond_stack.last ().second.first; + value_range *vr = cond_stack.last ().second.second; + value_range *vr_to_del = get_value_range (var); + XDELETE (vr_to_del); + change_value_range (var, vr); + cond_stack.pop (); + } that should be in after_dom_children I think and use a marker instead of a DOM query. See other examples like DOM itself or SCCVN. + /* Discover VR when condition is true. */ + if (te == e + && !TREE_OVERFLOW_P (op0) + && !TREE_OVERFLOW_P (op1)) you can use e->flags & EDGE_TRUE_VALUE/EDGE_FALSE_VALUE why do you need those TREE_OVERFLOW checks? + tree cond = build2 (code, boolean_type_node, op0, op1); + tree a = build2 (ASSERT_EXPR, TREE_TYPE (op0), op0, cond); + extract_range_from_assert (&vr, a); so I was hoping that the "refactoring" patch in the series would expose a more useful interface than extract_range_from_assert ... namely one that can extract a range from the comparison directly and does not require building a scratch ASSERT_EXPR. + /* If we found any usable VR, set the VR to ssa_name and create a + restore point in the cond_stack with the old VR. */ + if (vr.type == VR_RANGE || vr.type == VR_ANTI_RANGE) + { + value_range *new_vr = XCNEW (value_range); + *new_vr = vr; + cond_stack.safe_push (std::make_pair (bb, + std::make_pair (op0, + old_vr))); + change_value_range (op0, new_vr); I don't like 'change_value_range' as interface, please integrate that into a push/pop_value_range style interface instead. + vrp_visit_stmt (stmt, &taken_edge_p, &output_p); + } + + return NULL; you should return taken_edge_p (misnamed as it isn't a pointer) and take advantage of EDGE_EXECUTABLE. Again see DOM/SCCVN (might want to defer this as a followup improvement). Note that the advantage of a DOM-based VRP is that backtracking is easy to implement (you don't do that yet). That is, after DEF got a (better) value-range you can simply re-visit all the defs of its uses (and recursively). I think you have to be careful with stmts that might prematurely leave a BB though (like via EH). So sth for a followup as well. Thanks, Richard. > Thanks, > Kugan