From patchwork Wed Aug 10 09:14:29 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Kugan Vivekanandarajah X-Patchwork-Id: 73619 Delivered-To: patch@linaro.org Received: by 10.140.29.52 with SMTP id a49csp302318qga; Wed, 10 Aug 2016 02:15:06 -0700 (PDT) X-Received: by 10.66.156.226 with SMTP id wh2mr5372422pab.116.1470820506654; Wed, 10 Aug 2016 02:15:06 -0700 (PDT) Return-Path: Received: from sourceware.org (server1.sourceware.org. [209.132.180.131]) by mx.google.com with ESMTPS id j2si47611673paa.259.2016.08.10.02.15.06 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 10 Aug 2016 02:15:06 -0700 (PDT) Received-SPF: pass (google.com: domain of gcc-patches-return-433665-patch=linaro.org@gcc.gnu.org designates 209.132.180.131 as permitted sender) client-ip=209.132.180.131; Authentication-Results: mx.google.com; dkim=pass header.i=@gcc.gnu.org; spf=pass (google.com: domain of gcc-patches-return-433665-patch=linaro.org@gcc.gnu.org designates 209.132.180.131 as permitted sender) smtp.mailfrom=gcc-patches-return-433665-patch=linaro.org@gcc.gnu.org; dmarc=fail (p=NONE dis=NONE) header.from=linaro.org DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :subject:to:references:cc:from:message-id:date:mime-version :in-reply-to:content-type; q=dns; s=default; b=cB2ZRSMm0fMTJy7rj aRD18Q7RwzRXQRswPWWXN/jUn/sAsYMJVi3q9b02WFgfyKfPox1YGvWxlU3c7HF4 JP2TIE0O3pvF+5HjmNYpyJaUbOCY0juTSlaFKQYc2w7opohr5hBBVQVNpoZRXfeo pETpiLNMX+c7xJwmBUE1pwmR2U= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :subject:to:references:cc:from:message-id:date:mime-version :in-reply-to:content-type; s=default; bh=c9FLANUKpNsRo/FX/CyDrD1 XKVU=; b=XUYnKAc6QPmCN0tkbGcoTtMyUD5rflMEHpw3OzMrs98fG0mcZE0iVAC 5sk4iXUJQegwEJUYXzYTXRlJ6PZBzqgxIK6I54C+xrDpzjBGpbZwBx3y37jSN6xq lo6w63hBFm8590vvYGHIw/ksUA8iAENRGWq3UI+am70DK2X0GRAA= Received: (qmail 125054 invoked by alias); 10 Aug 2016 09:14:49 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Received: (qmail 125045 invoked by uid 89); 10 Aug 2016 09:14:48 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.4 required=5.0 tests=AWL, BAYES_00, RCVD_IN_DNSWL_LOW, SPF_PASS autolearn=ham version=3.3.2 spammy=sk:build_a, anonymous, H*MI:sk:2016081, Attached X-HELO: mail-pf0-f179.google.com Received: from mail-pf0-f179.google.com (HELO mail-pf0-f179.google.com) (209.85.192.179) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-GCM-SHA256 encrypted) ESMTPS; Wed, 10 Aug 2016 09:14:37 +0000 Received: by mail-pf0-f179.google.com with SMTP id y134so14290691pfg.0 for ; Wed, 10 Aug 2016 02:14:37 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:subject:to:references:cc:from:message-id:date :user-agent:mime-version:in-reply-to; bh=p8k7dEreuNvWzdBTktXcdPbbhGtSyxJBwY4GWJ0DoA0=; b=ZX3VwfqwxaKJStIcwTaGse05KWiZJDJjhmQC8BL8EWvHxdsPGT775wujhqMXqJvT70 ihHC5fiXyFDx062uyUnMnX7U+oZBRgpCJugB/UKRPb192H8F6btNuv4bMsal/KIlA5Kh 8nwybObyvfyRHPRs/gISLo7Ug+I6A5AqU9ItaMccukc0mJNJF2GkeX4LEzKkIt+PkfrV c3b3xvihFKP6LI56yqXpXcXnoX+0XPuoA0S6j17Ofwm/jA61MDYBgkwQzNG/gnOy2pLe eKWPhby+SrAki9bLIWdReyOnswv/BINCdW1sJ90u8SeUrpi3h5rvW3jdOujLel1OhRU/ 5zcA== X-Gm-Message-State: AEkoout6jPr3m+NIXPEFGmK475ELgCIWzIsTbOQ26aXl/ZhOf7c9VGXcFDkHZOlDPAM3le1J X-Received: by 10.98.92.65 with SMTP id q62mr5282109pfb.70.1470820475814; Wed, 10 Aug 2016 02:14:35 -0700 (PDT) Received: from [10.1.1.4] (58-6-183-210.dyn.iinet.net.au. [58.6.183.210]) by smtp.gmail.com with ESMTPSA id b134sm62010285pfb.55.2016.08.10.02.14.33 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 10 Aug 2016 02:14:35 -0700 (PDT) Subject: Re: [PR72835] Incorrect arithmetic optimization involving bitfield arguments To: Jakub Jelinek References: <0a1eaaf8-3ede-cd56-ffb5-40b25f94e46e@linaro.org> <98613cff-7c48-1a56-0014-6d87c35a8f26@linaro.org> <20160809214617.GB14857@tucnak.redhat.com> <7210cceb-be3b-44b1-13b7-4152e89d2a4f@linaro.org> <20160809215527.GC14857@tucnak.redhat.com> <0c53b0f3-4af6-387c-9350-95b1ae85850d@linaro.org> <20160810085703.GH14857@tucnak.redhat.com> Cc: "gcc-patches@gcc.gnu.org" , Richard Biener From: kugan Message-ID: <73f8e059-2c74-ad1d-97cc-3000f8374c2a@linaro.org> Date: Wed, 10 Aug 2016 19:14:29 +1000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0 MIME-Version: 1.0 In-Reply-To: <20160810085703.GH14857@tucnak.redhat.com> X-IsSubscribed: yes On 10/08/16 18:57, Jakub Jelinek wrote: > On Wed, Aug 10, 2016 at 08:51:32AM +1000, kugan wrote: >> I see it now. The problem is we are just looking at (-1) being in the ops >> list for passing changed to rewrite_expr_tree in the case of multiplication >> by negate. If we have combined (-1), as in the testcase, we will not have >> the (-1) and will pass changed=false to rewrite_expr_tree. >> >> We should set changed based on what happens in try_special_add_to_ops. >> Attached patch does this. Bootstrap and regression testing are ongoing. Is >> this OK for trunk if there is no regression. > > I think the bug is elsewhere. In particular in > undistribute_ops_list/zero_one_operation/decrement_power. > All those look problematic in this regard, they change RHS of statements > to something that holds a different value, while keeping the LHS. > So, generally you should instead just add a new stmt next to the old one, > and adjust data structures (replace the old SSA_NAME in some ->op with > the new one). decrement_power might be a problem here, dunno if all the > builtins are const in all cases that DSE would kill the old one, > Richard, any preferences for that? reset flow sensitive info + reset debug > stmt uses, or something different? Though, replacing the LHS with a new > anonymous SSA_NAME might be needed too, in case it is before SSA_NAME of a > user var that doesn't yet have any debug stmts. Hi Jakub, This is the patch I have now (not full tested yet). This is along what you described above. I think I have to handle all the LHS in zero_one_operation too, I will wait for the feedback before working on it. Thanks, Kugan diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr72835.c b/gcc/testsuite/gcc.dg/tree-ssa/pr72835.c index e69de29..049eddc 100644 --- a/gcc/testsuite/gcc.dg/tree-ssa/pr72835.c +++ b/gcc/testsuite/gcc.dg/tree-ssa/pr72835.c @@ -0,0 +1,36 @@ +/* PR tree-optimization/72835. */ +/* { dg-do run } */ +/* { dg-options "-O2" } */ + +struct struct_1 { + unsigned int m1 : 6 ; + unsigned int m2 : 24 ; + unsigned int m3 : 6 ; +}; + +unsigned short var_32 = 0x2d10; + +struct struct_1 s1; + +void init () +{ + s1.m1 = 4; + s1.m2 = 0x7ca4b8; + s1.m3 = 24; +} + +void foo () +{ + unsigned int c + = ((unsigned int) s1.m2) * (-((unsigned int) s1.m3)) + + (var_32) * (-((unsigned int) (s1.m1))); + if (c != 4098873984) + __builtin_abort (); +} + +int main () +{ + init (); + foo (); + return 0; +} diff --git a/gcc/tree-ssa-reassoc.c b/gcc/tree-ssa-reassoc.c index 7fd7550..b8dfa39 100644 --- a/gcc/tree-ssa-reassoc.c +++ b/gcc/tree-ssa-reassoc.c @@ -1039,7 +1039,7 @@ eliminate_using_constants (enum tree_code opcode, static void linearize_expr_tree (vec *, gimple *, - bool, bool); + bool, bool, bool *); /* Structure for tracking and counting operands. */ struct oecount { @@ -1183,9 +1183,25 @@ propagate_op_to_single_use (tree op, gimple *stmt, tree *def) is updated if there is only one operand but no operation left. */ static void -zero_one_operation (tree *def, enum tree_code opcode, tree op) +zero_one_operation (tree *def, enum tree_code opcode, tree op, bool ops_changed) { gimple *stmt = SSA_NAME_DEF_STMT (*def); + /* In this case, the result in the *def will be different as + compared to how it was. Therefore, to avoid having SSA + which will have range_info and debug that reflects old + operation, create a new SSA and use it (PR72835). */ + if (ops_changed) + { + use_operand_p use_p; + gimple *use_stmt; + gimple *stmt = SSA_NAME_DEF_STMT (*def); + tree new_def = make_ssa_name (TREE_TYPE (*def)); + gcc_checking_assert (single_imm_use (*def, &use_p, &use_stmt)); + SET_USE (use_p, new_def); + gimple_set_lhs (stmt, new_def); + *def = new_def; + update_stmt (use_stmt); + } do { @@ -1250,6 +1266,15 @@ zero_one_operation (tree *def, enum tree_code opcode, tree op) else if (is_gimple_assign (stmt2) && gimple_assign_rhs_code (stmt2) == NEGATE_EXPR) { + /* In this case the result in the op will be + different as compared to how it was. Therefore, to avoid + having SSA which will have range_info and debug that + reflects old operation, create a new SSA and use + it (PR72835). */ + tree tmp = make_ssa_name (TREE_TYPE (op)); + gimple_set_lhs (stmt2, tmp); + gimple_assign_set_rhs2 (stmt, tmp); + update_stmt (stmt2); if (gimple_assign_rhs1 (stmt2) == op) { tree cst = build_minus_one_cst (TREE_TYPE (op)); @@ -1453,7 +1478,8 @@ build_and_add_sum (tree type, tree op1, tree op2, enum tree_code opcode) static bool undistribute_ops_list (enum tree_code opcode, - vec *ops, struct loop *loop) + vec *ops, struct loop *loop, + bool *ops_changed) { unsigned int length = ops->length (); operand_entry *oe1; @@ -1521,7 +1547,7 @@ undistribute_ops_list (enum tree_code opcode, oedef = SSA_NAME_DEF_STMT ((*ops)[i]->op); oecode = gimple_assign_rhs_code (oedef); linearize_expr_tree (&subops[i], oedef, - associative_tree_code (oecode), false); + associative_tree_code (oecode), false, ops_changed); FOR_EACH_VEC_ELT (subops[i], j, oe1) { @@ -1617,7 +1643,7 @@ undistribute_ops_list (enum tree_code opcode, fprintf (dump_file, "Building ("); print_generic_expr (dump_file, oe1->op, 0); } - zero_one_operation (&oe1->op, c->oecode, c->op); + zero_one_operation (&oe1->op, c->oecode, c->op, *ops_changed); EXECUTE_IF_SET_IN_BITMAP (candidates2, first+1, i, sbi0) { gimple *sum; @@ -1627,7 +1653,7 @@ undistribute_ops_list (enum tree_code opcode, fprintf (dump_file, " + "); print_generic_expr (dump_file, oe2->op, 0); } - zero_one_operation (&oe2->op, c->oecode, c->op); + zero_one_operation (&oe2->op, c->oecode, c->op, *ops_changed); sum = build_and_add_sum (TREE_TYPE (oe1->op), oe1->op, oe2->op, opcode); oe2->op = build_zero_cst (TREE_TYPE (oe2->op)); @@ -4456,12 +4482,16 @@ acceptable_pow_call (gcall *stmt, tree *base, HOST_WIDE_INT *exponent) } /* Try to derive and add operand entry for OP to *OPS. Return false if - unsuccessful. */ + unsuccessful. If we changed the operands such that the (intermediate) + results can be different (as in the case of NEGATE_EXPR converted to + multiplication by -1), set ops_changed to true so that we will not + reuse the SSA (PR72835). */ static bool try_special_add_to_ops (vec *ops, enum tree_code code, - tree op, gimple* def_stmt) + tree op, gimple* def_stmt, + bool *ops_changed) { tree base = NULL_TREE; HOST_WIDE_INT exponent = 0; @@ -4492,6 +4522,8 @@ try_special_add_to_ops (vec *ops, add_to_ops_vec (ops, rhs1); add_to_ops_vec (ops, cst); gimple_set_visited (def_stmt, true); + if (ops_changed) + *ops_changed = true; return true; } @@ -4499,11 +4531,12 @@ try_special_add_to_ops (vec *ops, } /* Recursively linearize a binary expression that is the RHS of STMT. - Place the operands of the expression tree in the vector named OPS. */ + Place the operands of the expression tree in the vector named OPS. + Return TRUE if try_special_add_to_ops has set ops_changed to TRUE. */ static void linearize_expr_tree (vec *ops, gimple *stmt, - bool is_associative, bool set_visited) + bool is_associative, bool set_visited, bool *ops_changed) { tree binlhs = gimple_assign_rhs1 (stmt); tree binrhs = gimple_assign_rhs2 (stmt); @@ -4547,10 +4580,12 @@ linearize_expr_tree (vec *ops, gimple *stmt, if (!binrhsisreassoc) { - if (!try_special_add_to_ops (ops, rhscode, binrhs, binrhsdef)) + if (!try_special_add_to_ops (ops, rhscode, binrhs, + binrhsdef, ops_changed)) add_to_ops_vec (ops, binrhs); - if (!try_special_add_to_ops (ops, rhscode, binlhs, binlhsdef)) + if (!try_special_add_to_ops (ops, rhscode, binlhs, + binlhsdef, ops_changed)) add_to_ops_vec (ops, binlhs); return; @@ -4588,9 +4623,9 @@ linearize_expr_tree (vec *ops, gimple *stmt, || !is_reassociable_op (SSA_NAME_DEF_STMT (binrhs), rhscode, loop)); linearize_expr_tree (ops, SSA_NAME_DEF_STMT (binlhs), - is_associative, set_visited); + is_associative, set_visited, ops_changed); - if (!try_special_add_to_ops (ops, rhscode, binrhs, binrhsdef)) + if (!try_special_add_to_ops (ops, rhscode, binrhs, binrhsdef, ops_changed)) add_to_ops_vec (ops, binrhs); } @@ -5322,12 +5357,20 @@ reassociate_bb (basic_block bb) if (TREE_CODE (lhs) == SSA_NAME && has_zero_uses (lhs)) continue; + bool ops_changed = false; gimple_set_visited (stmt, true); - linearize_expr_tree (&ops, stmt, true, true); + linearize_expr_tree (&ops, stmt, true, true, NULL); ops.qsort (sort_by_operand_rank); optimize_ops_list (rhs_code, &ops); + /* While in undistribute_ops_list, NEGATE_EXPR is factored out, + operands to the reassociated stmts will be different + compared to how it was. In this case, to avoid having SSA + which will have range_info and debug that reflects old + operation, rewrite_expr_tree has to be called with + changed = true (PR72835). */ if (undistribute_ops_list (rhs_code, &ops, - loop_containing_stmt (stmt))) + loop_containing_stmt (stmt), + &ops_changed)) { ops.qsort (sort_by_operand_rank); optimize_ops_list (rhs_code, &ops); @@ -5415,7 +5458,8 @@ reassociate_bb (basic_block bb) new_lhs = rewrite_expr_tree (stmt, 0, ops, powi_result != NULL - || negate_result); + || negate_result + || ops_changed); } /* If we combined some repeated factors into a