From patchwork Mon Aug 16 14:21:48 2010 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Zbigniew Chamski X-Patchwork-Id: 61810 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) by ozlabs.org (Postfix) with SMTP id 1BFE9B70AE for ; Tue, 17 Aug 2010 00:22:05 +1000 (EST) Received: (qmail 19455 invoked by alias); 16 Aug 2010 14:22:02 -0000 Received: (qmail 19402 invoked by uid 22791); 16 Aug 2010 14:21:58 -0000 X-SWARE-Spam-Status: No, hits=-1.7 required=5.0 tests=AWL, BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, FREEMAIL_FROM, RCVD_IN_DNSWL_NONE X-Spam-Check-By: sourceware.org Received: from mail-gy0-f175.google.com (HELO mail-gy0-f175.google.com) (209.85.160.175) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Mon, 16 Aug 2010 14:21:51 +0000 Received: by gyd8 with SMTP id 8so2069010gyd.20 for ; Mon, 16 Aug 2010 07:21:49 -0700 (PDT) MIME-Version: 1.0 Received: by 10.100.174.16 with SMTP id w16mr5799016ane.266.1281968509327; Mon, 16 Aug 2010 07:21:49 -0700 (PDT) Received: by 10.231.13.194 with HTTP; Mon, 16 Aug 2010 07:21:48 -0700 (PDT) In-Reply-To: References: Date: Mon, 16 Aug 2010 16:21:48 +0200 Message-ID: Subject: Re: [patch] missing COND_EXPR case in walk_stmt_load_store_addr_ops From: Zbigniew Chamski To: Richard Guenther Cc: gcc-patches@gcc.gnu.org, Petros Panayi , Diego Novillo , sebpop@gmail.com 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 Hi Richard and all, I don't have a general (pure trunk-based and portable) test case yet. Given the current compilation flow, such a test case would require a way to trigger 'execute_update_address_taken' in late GIMPLE passes. The only way to do it is to have a portable builtin which is folded late and triggers the creation of new statements (cf. execute_fold_all_builtins() in tree-ssa-ccp.c and update_call_from_tree() in tree-ssa-propagate.c). Any other suggestions? BTW, there was a glitch in my original patch: the second arg of 'visit_addr()' calls in 'stmt_visit_select_addr_ops()' should have been wrapped in TREE_OPERAND(..., 0) Proof that this thing needs a proper regression test... Here's the corrected patch (retested on my experimental build and on the r163224 trunk). I changed the leading comment to emphasize that it's the operand of ADDR_EXPR that is supplied to 'visit_addr()'. Zbigniew 2010/8/16 Richard Guenther : > On Mon, Aug 16, 2010 at 1:57 PM, Zbigniew Chamski > wrote: >> All, >> >> walk_stmt_load_store_ops does not handle the case of COND_EXPR on the >> RHS of a GIMPLE assign statement, i.e., in 'selection assignments' of >> the form >> >>  v = [cond_expr] a RELOP b : x ? y; >> >> This may result in an ICE at SSA verification time if/when the >> ADDR_EXPRs inside the selection expression are not taken into account. >>  There is a corner case in libiberty (plus-demangle-template() in >> cplus-dem.c) where the only occurrence of an ADDR_TAKEN on variable >> 'pattern' ends up in a selection expression.  A call to >> 'execute_update_addresses_taken()' will miss it, resulting in an >> "address taken, but ADDRESSABLE bit not set" error at SSA verification >> time.  This normally goes unnoticed because there are no calls to >> 'execute_update_addresses_taken()' after SRA and the COND_EXPRs appear >> on the RHS much later, during LIM.  However, the bug will be triggered >> if/when execute_fold_all_builtins  (or a custom pass) alters data >> addressing and sets TODO_update_address_taken.  In my case it was >> uncovered by a custom pass. >> >> The root cause is in the logic of 'walk_stmt_load_store_ops' which >> lacks the COND_EXPR case for simple assigns.  A proposed patch is >> below, bootstrapped and tested (C & C++) on x86_64-unknown-linux-gnu. >> >> Comments welcome! > > The patch is ok for trunk and the 4.5 branch (do you have a testcase?). > > Thanks, > Richard. > >>    Zbigniew >> >> Index: gcc/gimple.c >> =================================================================== >> --- gcc/gimple.c        (revision 163224) >> +++ gcc/gimple.c        (working copy) >> @@ -4746,6 +4746,45 @@ >>   return NULL_TREE; >>  } >> >> + >> +/* For an assignment STMT which contains a selection expression on the RHS, >> +   i.e., >> + >> +     lhs = [cond_expr] a RELOP b ? x : y; >> + >> +   search the entire selection expression for occurrences of ADDR_EXPR, and >> +   if found, apply VISIT_ADDR callback on STMT, the corresponding ADDR_EXPR >> +   operand and DATA.  Return TRUE if any of the callback invocations >> +   returned TRUE.  */ >> + >> +static inline bool >> +stmt_visit_select_addr_ops (gimple stmt, tree select_expr, void *data, >> +                           bool (*visit_addr)(gimple, tree, void *)) >> +{ >> +  bool ret = false; >> +  tree condop1, condop2; >> +  tree trueval, falseval; >> + >> +  condop1 = TREE_OPERAND (TREE_OPERAND (select_expr, 0), 0); >> +  if (TREE_CODE (condop1) == ADDR_EXPR) >> +    ret |= visit_addr (stmt, condop1, data); >> + >> +  condop2 = TREE_OPERAND (TREE_OPERAND (select_expr, 1), 0); >> +  if (TREE_CODE (condop2) == ADDR_EXPR) >> +    ret |= visit_addr (stmt, condop2, data); >> + >> +  trueval = TREE_OPERAND (select_expr, 1); >> +  if (TREE_CODE (trueval) == ADDR_EXPR) >> +    ret |= visit_addr (stmt, trueval, data); >> + >> +  falseval = TREE_OPERAND (select_expr, 2); >> +  if (TREE_CODE (falseval) == ADDR_EXPR) >> +    ret |= visit_addr (stmt, falseval, data); >> + >> +  return ret; >> +} >> + >> + >>  /* For the statement STMT call the callbacks VISIT_LOAD, VISIT_STORE and >>    VISIT_ADDR if non-NULL on loads, store and address-taken operands >>    passing the STMT, the base of the operand and DATA to it.  The base >> @@ -4761,6 +4800,7 @@ >>  { >>   bool ret = false; >>   unsigned i; >> + >>   if (gimple_assign_single_p (stmt)) >>     { >>       tree lhs, rhs; >> @@ -4785,6 +4825,9 @@ >>                   && TREE_CODE (OBJ_TYPE_REF_OBJECT (rhs)) == ADDR_EXPR) >>            ret |= visit_addr (stmt, TREE_OPERAND (OBJ_TYPE_REF_OBJECT (rhs), >>                                                   0), data); >> +         else if (TREE_CODE (rhs) == COND_EXPR) >> +           ret |= stmt_visit_select_addr_ops (stmt, rhs, data, visit_addr); >> + >>           lhs = gimple_assign_lhs (stmt); >>          if (TREE_CODE (lhs) == TARGET_MEM_REF >>               && TMR_BASE (lhs) != NULL_TREE >> > Index: gcc/gimple.c =================================================================== --- gcc/gimple.c (revision 163224) +++ gcc/gimple.c (working copy) @@ -4746,6 +4746,45 @@ return NULL_TREE; } + +/* For an assignment STMT which contains a selection expression on the RHS, + i.e., + + lhs = [cond_expr] a RELOP b ? x : y; + + search the entire selection expression for occurrences of ADDR_EXPR, and + if found, apply VISIT_ADDR callback on STMT, the corresponding operand + of ADDR_EXPR and DATA. Return TRUE if any of the callback invocations + returned TRUE. */ + +static inline bool +stmt_visit_select_addr_ops (gimple stmt, tree select_expr, void *data, + bool (*visit_addr)(gimple, tree, void *)) +{ + bool ret = false; + tree condop1, condop2; + tree trueval, falseval; + + condop1 = TREE_OPERAND (TREE_OPERAND (select_expr, 0), 0); + if (TREE_CODE (condop1) == ADDR_EXPR) + ret |= visit_addr (stmt, TREE_OPERAND (condop1, 0), data); + + condop2 = TREE_OPERAND (TREE_OPERAND (select_expr, 0), 1); + if (TREE_CODE (condop2) == ADDR_EXPR) + ret |= visit_addr (stmt, TREE_OPERAND (condop2, 0), data); + + trueval = TREE_OPERAND (select_expr, 1); + if (TREE_CODE (trueval) == ADDR_EXPR) + ret |= visit_addr (stmt, TREE_OPERAND (trueval, 0), data); + + falseval = TREE_OPERAND (select_expr, 2); + if (TREE_CODE (falseval) == ADDR_EXPR) + ret |= visit_addr (stmt, TREE_OPERAND (falseval, 0), data); + + return ret; +} + + /* For the statement STMT call the callbacks VISIT_LOAD, VISIT_STORE and VISIT_ADDR if non-NULL on loads, store and address-taken operands passing the STMT, the base of the operand and DATA to it. The base @@ -4761,6 +4800,7 @@ { bool ret = false; unsigned i; + if (gimple_assign_single_p (stmt)) { tree lhs, rhs; @@ -4785,6 +4825,9 @@ && TREE_CODE (OBJ_TYPE_REF_OBJECT (rhs)) == ADDR_EXPR) ret |= visit_addr (stmt, TREE_OPERAND (OBJ_TYPE_REF_OBJECT (rhs), 0), data); + else if (TREE_CODE (rhs) == COND_EXPR) + ret |= stmt_visit_select_addr_ops (stmt, rhs, data, visit_addr); + lhs = gimple_assign_lhs (stmt); if (TREE_CODE (lhs) == TARGET_MEM_REF && TMR_BASE (lhs) != NULL_TREE