From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 25320 invoked by alias); 19 Aug 2016 11:41:26 -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 25304 invoked by uid 89); 19 Aug 2016 11:41:25 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.5 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,RCVD_IN_DNSWL_LOW,SPF_PASS autolearn=ham version=3.3.2 spammy=sk:bitmap_, respective, Discover X-HELO: mail-wm0-f53.google.com Received: from mail-wm0-f53.google.com (HELO mail-wm0-f53.google.com) (74.125.82.53) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 19 Aug 2016 11:41:15 +0000 Received: by mail-wm0-f53.google.com with SMTP id o80so36668556wme.1 for ; Fri, 19 Aug 2016 04:41:14 -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=D2S550JQRuY1AjAx9ZEDNvol0sy6WI/ACG2cyywvKhY=; b=ZF9yxI+xBIuHdz4FVnujVq7gb4v1SgqHEWpz1GCsMv9Xpax/MPeiZ/jEWxRiJ9SRNZ +3zDqalTKv6qEq7s7ITH79RkTRizT67Fa2zrBq3e047ZmWekpkX/rjj7da/97D3Jur1E OLCkC5tNGLo/nTeWsxhkmlT6Acde4PJ8XNbQDWBI0eoyvvqBkI62z7qfx7kVQQ4Tx+rT VfyFPeA7jFxbqg99i+h9corxIQjeks6qDwDSBepCx6Qb4SFv8UoJji7D3U3uzvD4gvZL 0v6MdGUW9I5AgewglnthPxSfJ3PFVWoGwJVvzNot7P797ppqa/clewsB1Jw6H+1Mceze XxkA== X-Gm-Message-State: AEkooutuGbxGN5IMRxNs8XB7uXxf0Cubo2xMlIRyPfD3JgyycRBPvQW7qRCu3T2Yo9qfHWCffQr7eogYPsBU4A== X-Received: by 10.194.107.33 with SMTP id gz1mr5971520wjb.178.1471606872858; Fri, 19 Aug 2016 04:41:12 -0700 (PDT) MIME-Version: 1.0 Received: by 10.28.137.202 with HTTP; Fri, 19 Aug 2016 04:41:11 -0700 (PDT) In-Reply-To: References: <57886949.8010300@linaro.org> <57886A71.6030103@linaro.org> <57888BD6.6070200@linaro.org> <578891C8.7090609@linaro.org> <19ff8188-aed7-0f9e-cc0b-0603698787ff@linaro.org> <48e42d0c-057c-312a-4e41-cd78c8b38b5e@linaro.org> From: Richard Biener Date: Fri, 19 Aug 2016 11:41: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-08/txt/msg01403.txt.bz2 On Tue, Aug 16, 2016 at 9:45 AM, kugan wrote: > Hi Richard, > > On 12/08/16 20:43, Richard Biener wrote: >> >> On Wed, Aug 3, 2016 at 3:17 AM, kugan >> wrote: > > > [SNIP] > >> >> diff --git a/gcc/common.opt b/gcc/common.opt >> index 8a292ed..7028cd4 100644 >> --- a/gcc/common.opt >> +++ b/gcc/common.opt >> @@ -2482,6 +2482,10 @@ ftree-vrp >> Common Report Var(flag_tree_vrp) Init(0) Optimization >> Perform Value Range Propagation on trees. >> >> +fdisable-tree-evrp >> +Common Report Var(flag_disable_early_vrp) Init(0) Optimization >> +Disable Early Value Range Propagation on trees. >> + >> >> no please, this is automatically supported via -fdisable- > > > I am now having -ftree-evrp which is enabled all the time. But This will > only be used for disabling the early-vrp. That is, early-vrp will be run > when ftree-vrp is enabled and ftree-evrp is not explicitly disabled. Is this > OK? Why would one want to disable early-vrp? I see you do this in the testsuite for non-early VRP unit-tests but using -fdisable-tree-evrp1 there would be ok as well. >> >> @@ -1728,11 +1736,12 @@ extract_range_from_assert (value_range *vr_p, tree >> expr) >> always false. */ >> >> static void >> -extract_range_from_ssa_name (value_range *vr, tree var) >> +extract_range_from_ssa_name (value_range *vr, bool dom_p, tree var) >> { >> value_range *var_vr = get_value_range (var); >> >> - if (var_vr->type != VR_VARYING) >> + if (var_vr->type != VR_VARYING >> + && (!dom_p || var_vr->type != VR_UNDEFINED)) >> copy_value_range (vr, var_vr); >> else >> set_value_range (vr, VR_RANGE, var, var, NULL); >> >> why do you need these changes? I think I already told you you need to >> initialize the lattice to sth else than VR_UNDEFINED and that you can't >> fully re-use update_value_range. If you don't want to do that then >> instead >> of doing changes all over the place do it in get_value_range and have a >> global flag. > > > I have now added a global early_vrp_p and use this to initialize > VR_INITIALIZER and get_value_range default to VR_VARYING. ICK. Ok, I see that this works, but it is quite ugly, so (see below) >> >> >> @@ -3594,7 +3643,8 @@ extract_range_from_cond_expr (value_range *vr, >> gassign *stmt) >> on the range of its operand and the expression code. */ >> >> static void >> -extract_range_from_comparison (value_range *vr, enum tree_code code, >> +extract_range_from_comparison (value_range *vr, >> + enum tree_code code, >> tree type, tree op0, tree op1) >> { >> bool sop = false; >> >> remove these kind of no-op changes. > > > Done. > > >> >> +/* Initialize local data structures for VRP. If DOM_P is true, >> + we will be calling this from early_vrp where value range propagation >> + is done by visiting stmts in dominator tree. ssa_propagate engine >> + is not used in this case and that part of the ininitialization will >> + be skipped. */ >> >> static void >> -vrp_initialize (void) >> +vrp_initialize (bool dom_p) >> { >> basic_block bb; >> >> @@ -6949,6 +7010,9 @@ vrp_initialize (void) >> vr_phi_edge_counts = XCNEWVEC (int, num_ssa_names); >> bitmap_obstack_initialize (&vrp_equiv_obstack); >> >> + if (dom_p) >> + return; >> + >> >> split the function instead. >> >> @@ -7926,7 +7992,8 @@ vrp_visit_switch_stmt (gswitch *stmt, edge >> *taken_edge_p) >> If STMT produces a varying value, return SSA_PROP_VARYING. */ >> >> static enum ssa_prop_result >> -vrp_visit_stmt (gimple *stmt, edge *taken_edge_p, tree *output_p) >> +vrp_visit_stmt_worker (gimple *stmt, bool dom_p, edge *taken_edge_p, >> + tree *output_p) >> { >> tree def; >> ssa_op_iter iter; >> @@ -7940,7 +8007,7 @@ vrp_visit_stmt (gimple *stmt, edge >> *taken_edge_p, tree *output_p) >> if (!stmt_interesting_for_vrp (stmt)) >> gcc_assert (stmt_ends_bb_p (stmt)); >> else if (is_gimple_assign (stmt) || is_gimple_call (stmt)) >> - return vrp_visit_assignment_or_call (stmt, output_p); >> + return vrp_visit_assignment_or_call (stmt, dom_p, output_p); >> else if (gimple_code (stmt) == GIMPLE_COND) >> return vrp_visit_cond_stmt (as_a (stmt), taken_edge_p); >> else if (gimple_code (stmt) == GIMPLE_SWITCH) >> @@ -7954,6 +8021,12 @@ vrp_visit_stmt (gimple *stmt, edge >> *taken_edge_p, tree *output_p) >> return SSA_PROP_VARYING; >> } >> >> +static enum ssa_prop_result >> +vrp_visit_stmt (gimple *stmt, edge *taken_edge_p, tree *output_p) >> +{ >> + return vrp_visit_stmt_worker (stmt, false, taken_edge_p, output_p); >> +} >> >> as said the refactoring that would be appreciated is to split out the >> update_value_range calls >> from the worker functions so you can call the respective functions >> from the DOM implementations. >> That they are globbed in vrp_visit_stmt currently is due to the API of >> the SSA propagator. > > Sent this as separate patch for easy reviewing and testing. Thanks. >> >> @@ -8768,6 +8842,12 @@ vrp_visit_phi_node (gphi *phi) >> fprintf (dump_file, "\n"); >> } >> >> + if (dom_p && vr_arg.type == VR_UNDEFINED) >> + { >> + set_value_range_to_varying (&vr_result); >> + break; >> + } >> + >> >> eh... ok, so another way to attack this is, instead of initializing >> the lattice to sth else >> than VR_UNDEFINED, make sure to drop the lattice to varying for all PHI >> args on >> yet unvisited incoming edges (you're not doing optimistic VRP). That's >> the only >> place you _have_ to do it. > > > Even when it is initialize (as I am doing now), we can still end up with > VR_UNDEFINED during the propagation. I have just left the above so that we > dont end up with the wrong VR. How would we end up with wrong VRs? I see + /* Discover VR when condition is true. */ + extract_range_for_var_from_comparison_expr (op0, code, op0, op1, &vr); + if (old_vr->type == VR_RANGE || old_vr->type == VR_ANTI_RANGE) + vrp_intersect_ranges (&vr, old_vr); where you already disregard UNDEFINED as old_vr. What you might be missing is to set all DEFs on !stmt_interesting_for_vrp to VARYING. Also + if (stmt_interesting_for_vrp (stmt)) + { + tree lhs = gimple_get_lhs (stmt); + value_range vr = VR_INITIALIZER; + vrp_visit_stmt_worker (stmt, &taken_edge, &output, &vr); + update_value_range (lhs, &vr); + + /* Try folding stmts with the VR discovered. */ + if (fold_stmt (&gsi, follow_single_use_edges)) + update_stmt (gsi_stmt (gsi)); folding here can introduce additional stmts and thus unvisited SSA defs which would end up with VR_UNINITIALIZED defs - I don't see exactly where that would cause issues but I'm not sure it might not. So better use fold_stmt_inplace or alternatively if fold_stmt returned true then, remembering the previous stmt gsi, iterate over all stmts emitted and set their defs to varying (or re-visit them). See for example how tree-ssa-forwprop.c does this re-visiting (it revisits all changed stmts). Note that you want to have a custom valueize function instead of just follow_single_use_edges as you want to valueize all SSA names according to their lattice value (if it has a single value). You can use vrp_valueize for this though that gets you non-single-use edge following as well. Eventually it's going to be cleaner to do what the SSA propagator does and before folding do did_replace = replace_uses_in (stmt, vrp_valueize); if (fold_stmt (&gsi, follow_single_use_edges) || did_replace) update_stmt (gsi_stmt (gsi)); exporting replace_uses_in for this is ok. I guess I prefer this for now. Overall this now looks good apart from the folding and the VR_INITIALIZER thing. You can commit the already approved refactoring changes and combine this patch with the struct value_range move, this way I can more easily look into issues with the UNDEFINED thing if you can come up with a testcase that doesn't work. Thanks, Richard. > I also noticed that g++.dg/warn/pr33738.C testcase is now failing. This is > because, with early-vrp setting value range ccp2 is optimizing without > issuing a warning. I will look into it. > > bootstrap and regression testing is in progress. > > Thanks, > Kugan