From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 28756 invoked by alias); 14 Sep 2019 17:18:10 -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 28747 invoked by uid 89); 14 Sep 2019 17:18:10 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-5.1 required=5.0 tests=AWL,BAYES_00 autolearn=ham version=3.3.1 spammy=FBI, sk:SSA_NAM, work, sk:ssa_nam X-HELO: nikam.ms.mff.cuni.cz Received: from nikam.ms.mff.cuni.cz (HELO nikam.ms.mff.cuni.cz) (195.113.20.16) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Sat, 14 Sep 2019 17:18:08 +0000 Received: by nikam.ms.mff.cuni.cz (Postfix, from userid 16202) id 08E24280FD3; Sat, 14 Sep 2019 19:18:05 +0200 (CEST) Date: Sat, 14 Sep 2019 17:18:00 -0000 From: Jan Hubicka To: Feng Xue OS Cc: Martin Jambor , "gcc-patches@gcc.gnu.org" Subject: Re: [PATCH V3] Generalized predicate/condition for parameter reference in IPA (PR ipa/91088) Message-ID: <20190914171804.xept5u4gszujsp7r@kam.mff.cuni.cz> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: NeoMutt/20170113 (1.7.2) X-SW-Source: 2019-09/txt/msg00899.txt.bz2 > +/* Analyze EXPR if it represents a series of simple operations performed on > + a function parameter and return true if so. FBI, STMT, EXPR, INDEX_P and > + AGGPOS have the same meaning like in unmodified_parm_or_parm_agg_item. > + Type of the parameter or load from an aggregate via the parameter is > + stored in *TYPE_P. Operations on the parameter are recorded to > + PARAM_OPS_P if it is not NULL. */ > + > +static bool > +decompose_param_expr (struct ipa_func_body_info *fbi, > + gimple *stmt, tree expr, > + int *index_p, tree *type_p, > + struct agg_position_info *aggpos, > + expr_eval_ops *param_ops_p = NULL) > +{ > + int op_limit = PARAM_VALUE (PARAM_IPA_MAX_PARAM_EXPR_OPS); > + int op_count = 0; > + > + if (param_ops_p) > + *param_ops_p = NULL; > + > + while (true) > + { > + expr_eval_op eval_op; > + poly_int64 size; > + > + if (unmodified_parm_or_parm_agg_item (fbi, stmt, expr, index_p, &size, > + aggpos)) > + { > + tree type = TREE_TYPE (expr); > + > + /* Stop if found bit-field whose size does not equal any natural > + integral type. */ > + if (maybe_ne (tree_to_poly_int64 (TYPE_SIZE (type)), size)) > + break; Why is this needed? > + > + *type_p = type; > + return true; > + } > + > + if (TREE_CODE (expr) != SSA_NAME || SSA_NAME_IS_DEFAULT_DEF (expr)) > + break; > + > + if (!is_gimple_assign (stmt = SSA_NAME_DEF_STMT (expr))) > + break; > + > + switch (gimple_assign_rhs_class (stmt)) > + { > + case GIMPLE_SINGLE_RHS: > + expr = gimple_assign_rhs1 (stmt); > + continue; > + > + case GIMPLE_UNARY_RHS: > + expr = gimple_assign_rhs1 (stmt); > + > + eval_op.val_is_rhs = false; I find val_is_rhs to be confusing, lhs/rhs is usually used for the gimple assignments. Here everything is rhs, but it depends whether the val is operand0 or operand1. So I guess we could do val_is_op1. > @@ -1183,10 +1301,8 @@ set_cond_stmt_execution_predicate (struct ipa_func_body_info *fbi, > if (!is_gimple_ip_invariant (gimple_cond_rhs (last))) > return; > op = gimple_cond_lhs (last); > - /* TODO: handle conditionals like > - var = op0 < 4; > - if (var != 0). */ > - if (unmodified_parm_or_parm_agg_item (fbi, last, op, &index, &size, &aggpos)) > + > + if (decompose_param_expr (fbi, last, op, &index, &type, &aggpos, ¶m_ops)) > { > code = gimple_cond_code (last); > inverted_code = invert_tree_comparison (code, HONOR_NANS (op)); > @@ -1201,13 +1317,13 @@ set_cond_stmt_execution_predicate (struct ipa_func_body_info *fbi, > if (this_code != ERROR_MARK) > { > predicate p > - = add_condition (summary, index, size, &aggpos, this_code, > - unshare_expr_without_location > - (gimple_cond_rhs (last))); > + = add_condition (summary, index, type, &aggpos, this_code, > + gimple_cond_rhs (last), param_ops); An why unsharing is no longer needed here? It is importnat to avoid anything which reffers to function local blocks to get into the global LTO stream. > +/* Check whether two set of operations have same effects. */ > +static bool > +expr_eval_ops_equal_p (expr_eval_ops ops1, expr_eval_ops ops2) > +{ > + if (ops1) > + { > + if (!ops2 || ops1->length () != ops2->length ()) > + return false; > + > + for (unsigned i = 0; i < ops1->length (); i++) > + { > + expr_eval_op &op1 = (*ops1)[i]; > + expr_eval_op &op2 = (*ops2)[i]; > + > + if (op1.code != op2.code > + || op1.val_is_rhs != op2.val_is_rhs > + || !vrp_operand_equal_p (op1.val, op2.val) Why you need vrp_operand_equal_p instead of operand_equal_p here? Overall the patch looks very reasonable. We may end up with bit more general expressions (i.e. supporting arithmetics involving more than one operand). I see you also fixed a lot of typos in comments, please go head and commit them separately as obvious. Thank for all the work! Honza