From patchwork Tue Mar 26 21:10:34 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: "Iyer, Balaji V" X-Patchwork-Id: 231533 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]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (Client CN "localhost", Issuer "www.qmailtoaster.com" (not verified)) by ozlabs.org (Postfix) with ESMTPS id 858E62C008E for ; Wed, 27 Mar 2013 08:11:29 +1100 (EST) DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:from :to:cc:subject:date:message-id:references:in-reply-to :content-type:mime-version; q=dns; s=default; b=cT65pEhdx6egwEJW Uhu2DNnYIs9YhnmcNYaF80nNQ4vFlq7miW+B4qPG5yv/ivVTX8c6jFvCEKfVOKiV 6QL00hfFAsYFONR/iRs6dQkw956Q9OP1DqFY25W8rTA2oDWWbn0/2QoPQVYRNeGK s0Po+AUuFoUClwiFRomY9PBEbNY= 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:from :to:cc:subject:date:message-id:references:in-reply-to :content-type:mime-version; s=default; bh=NeG2fsQo98ZIF93Xa3G/hx UkDR8=; b=MF7wpnCY8XSOsUoxL11wCswxdSFeLRgkWkhVD5p1hZlrhMgazSLwa2 km4D+hzEaZhQyydb1a5I5XxCLeXx1IfUkt1GGYBJqSzj/6caOX0wgPaCPFdiSwSu R3RaTn+0UDjz+gLSTGXd7bZC3xhdcbLO4u5b6jrJEMWNNvZvPBoPU= Received: (qmail 2275 invoked by alias); 26 Mar 2013 21:11:18 -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 2225 invoked by uid 89); 26 Mar 2013 21:11:09 -0000 X-Spam-SWARE-Status: No, score=-6.4 required=5.0 tests=AWL, BAYES_00, KHOP_RCVD_UNTRUST, KHOP_THREADED, RCVD_IN_DNSWL_HI, RCVD_IN_HOSTKARMA_W, RP_MATCHES_RCVD autolearn=ham version=3.3.1 Received: from mga14.intel.com (HELO mga14.intel.com) (143.182.124.37) by sourceware.org (qpsmtpd/0.84/v0.84-167-ge50287c) with ESMTP; Tue, 26 Mar 2013 21:11:06 +0000 Received: from azsmga002.ch.intel.com ([10.2.17.35]) by azsmga102.ch.intel.com with ESMTP; 26 Mar 2013 14:11:03 -0700 X-ExtLoop1: 1 Received: from fmsmsx107.amr.corp.intel.com ([10.19.9.54]) by AZSMGA002.ch.intel.com with ESMTP; 26 Mar 2013 14:10:57 -0700 Received: from fmsmsx102.amr.corp.intel.com ([169.254.2.241]) by FMSMSX107.amr.corp.intel.com ([169.254.9.210]) with mapi id 14.01.0355.002; Tue, 26 Mar 2013 14:10:34 -0700 From: "Iyer, Balaji V" To: Joseph Myers , Aldy Hernandez CC: "gcc-patches@gcc.gnu.org" Subject: RE: [patch] cilkplus array notation for C (clean, independent patchset, take 1) Date: Tue, 26 Mar 2013 21:10:34 +0000 Message-ID: References: <5149D62F.9070503@redhat.com> <51507F31.1080003@redhat.com> In-Reply-To: MIME-Version: 1.0 X-Virus-Found: No Hello Joseph, Aldy et al., Attached please find a patch that will fixed the problem below and another problem you mentioned in a previous email (I had used really_constant_p(..) and you mentioned that in C we need to check for INTEGER_CST). Please let me know if I have missed anything else that you mentioned. Thanks, Balaji V. Iyer. > -----Original Message----- > From: Joseph Myers [mailto:joseph@codesourcery.com] > Sent: Tuesday, March 26, 2013 1:05 PM > To: Aldy Hernandez > Cc: Iyer, Balaji V; gcc-patches@gcc.gnu.org > Subject: Re: [patch] cilkplus array notation for C (clean, independent patchset, > take 1) > > On Mon, 25 Mar 2013, Aldy Hernandez wrote: > > > > I always tend to check for a null pointer before I access the fields > > > in the structure. In this case it is unnecessary. In some cases > > > (e.g. find_rank) there is a good chance a null pointer will be > > > passed into the function and we need to check that and reject those. > > > > I think what Joseph is suggesting is that if NULL is not valid, then > > the caller should check this. But if NULL is valid, then it should be > > documented in the function comment at the top. > > The caller should only check it if it's valid in the caller but not the callee. If it's > invalid in the caller as well, neither should check (except maybe in an assertion if > felt appropriate in a particular case). FIXED! > > -- > Joseph S. Myers > joseph@codesourcery.com diff --git a/gcc/c-family/array-notation-common.c b/gcc/c-family/array-notation-common.c index b775225..894c02a 100644 --- a/gcc/c-family/array-notation-common.c +++ b/gcc/c-family/array-notation-common.c @@ -152,7 +152,7 @@ extract_sec_implicit_index_arg (location_t location, tree fn) if (TREE_CODE (fn) == CALL_EXPR) { fn_arg = CALL_EXPR_ARG (fn, 0); - if (really_constant_p (fn_arg)) + if (TREE_CODE (fn_arg) == INTEGER_CST) return_int = int_cst_value (fn_arg); else { diff --git a/gcc/c/c-array-notation.c b/gcc/c/c-array-notation.c index 9ee281e..dd0a1b0 100755 --- a/gcc/c/c-array-notation.c +++ b/gcc/c/c-array-notation.c @@ -52,9 +52,7 @@ struct inv_list /* Set *RANK of expression ARRAY, ignoring array notation specific built-in functions if IGNORE_BUILTIN_FN is true. The ORIG_EXPR is printed out if an error occured in the rank calculation. The functions returns false if it - encounters an error in rank calculation. If ARRAY can be NULL, since it is - recursively accessing all the fields in a subtree. If so, then just return - true. + encounters an error in rank calculation. For example, an array notation of A[:][:] or B[0:10][0:5:2] or C[5][:][1:0] all have a rank of 2. */ @@ -66,10 +64,8 @@ find_rank (location_t loc, tree orig_expr, tree array, bool ignore_builtin_fn, tree ii_tree; size_t ii = 0, current_rank = 0; an_reduce_type dummy_type = REDUCE_UNKNOWN; - - if (!array) - return true; - else if (TREE_CODE (array) == ARRAY_NOTATION_REF) + + if (TREE_CODE (array) == ARRAY_NOTATION_REF) { for (ii_tree = array; ii_tree && TREE_CODE (ii_tree) == ARRAY_NOTATION_REF; @@ -111,7 +107,8 @@ find_rank (location_t loc, tree orig_expr, tree array, bool ignore_builtin_fn, } else for (ii = 0; ii < TREE_CODE_LENGTH (TREE_CODE (array)); ii++) - if (!find_rank (loc, orig_expr, TREE_OPERAND (array, ii), + if (TREE_OPERAND (array, ii) + && !find_rank (loc, orig_expr, TREE_OPERAND (array, ii), ignore_builtin_fn, rank)) return false; } @@ -133,21 +130,22 @@ extract_array_notation_exprs (tree node, bool ignore_builtin_fn, size_t ii = 0; an_reduce_type dummy_type = REDUCE_UNKNOWN; - if (!node) - return; - else if (TREE_CODE (node) == ARRAY_NOTATION_REF) + if (TREE_CODE (node) == ARRAY_NOTATION_REF) { vec_safe_push (*array_list, node); return; } else if (TREE_CODE (node) == TREE_LIST) { - extract_array_notation_exprs (TREE_PURPOSE (node), ignore_builtin_fn, - array_list); - extract_array_notation_exprs (TREE_VALUE (node), ignore_builtin_fn, - array_list); - extract_array_notation_exprs (TREE_CHAIN (node), ignore_builtin_fn, - array_list); + if (TREE_PURPOSE (node)) + extract_array_notation_exprs (TREE_PURPOSE (node), ignore_builtin_fn, + array_list); + if (TREE_VALUE (node)) + extract_array_notation_exprs (TREE_VALUE (node), ignore_builtin_fn, + array_list); + if (TREE_CHAIN (node)) + extract_array_notation_exprs (TREE_CHAIN (node), ignore_builtin_fn, + array_list); } else if (TREE_CODE (node) == STATEMENT_LIST) { @@ -181,8 +179,9 @@ extract_array_notation_exprs (tree node, bool ignore_builtin_fn, } else for (ii = 0; ii < TREE_CODE_LENGTH (TREE_CODE (node)); ii++) - extract_array_notation_exprs (TREE_OPERAND (node, ii), - ignore_builtin_fn, array_list); + if (TREE_OPERAND (node, ii)) + extract_array_notation_exprs (TREE_OPERAND (node, ii), + ignore_builtin_fn, array_list); return; } @@ -190,10 +189,7 @@ extract_array_notation_exprs (tree node, bool ignore_builtin_fn, /* Replaces all the occurances of array notations in *LIST with the appropriate one in ARRAY_OPERAND. If IGNORE_BUILTIN_FN is set, then array notations inside array-notation specific builtin functions are ignored. ORIG can be - anything from a collection of statement lists to a single variable. Since - this function steps through all the subtrees, it is probable that *ORIG can - be a NULL_TREE. If so, then the function just returns. -*/ + anything from a collection of statement lists to a single variable. */ static void replace_array_notations (tree *orig, bool ignore_builtin_fn, @@ -204,7 +200,7 @@ replace_array_notations (tree *orig, bool ignore_builtin_fn, tree node = NULL_TREE, node_replacement = NULL_TREE; an_reduce_type dummy_type = REDUCE_UNKNOWN; - if (vec_safe_length (list) == 0 || !*orig) + if (vec_safe_length (list) == 0) return; if (TREE_CODE (*orig) == ARRAY_NOTATION_REF) @@ -262,8 +258,9 @@ replace_array_notations (tree *orig, bool ignore_builtin_fn, else { for (ii = 0; ii < (size_t) TREE_CODE_LENGTH (TREE_CODE (*orig)); ii++) - replace_array_notations (&TREE_OPERAND (*orig, ii), ignore_builtin_fn, - list, array_operand); + if (TREE_OPERAND (*orig, ii)) + replace_array_notations (&TREE_OPERAND (*orig, ii), ignore_builtin_fn, + list, array_operand); } return; } @@ -330,9 +327,7 @@ replace_inv_trees (tree *tp, int *walk_subtrees, void *data) /* Replaces all the scalar expressions in *NODE. Returns a STATEMENT_LIST that holds the NODE along with variables that holds the results of the invariant - expressions. Since this function steps through all the subtrees, it is - probable than *NODE or NODE can be NULL_TREE and NULL respectively. If so, - then the function just returns NULL_TREE. */ + expressions. */ tree replace_invariant_exprs (tree *node) @@ -342,9 +337,6 @@ replace_invariant_exprs (tree *node) tree t = NULL_TREE, new_var = NULL_TREE, new_node; struct inv_list data; - if (!node || !*node) - return NULL_TREE; - data.list_values = NULL; data.replacement = NULL; walk_tree (node, find_inv_trees, (void *)&data, NULL); @@ -987,6 +979,8 @@ build_array_notation_expr (location_t location, tree lhs, tree lhs_origtype, build2 (PLUS_EXPR, TREE_TYPE (lhs_var[ii]), lhs_var[ii], build_one_cst (TREE_TYPE (lhs_var[ii])))); + /* If array_expr_lhs is NULL, then we have function that returns void or + its return value is ignored. */ if (!array_expr_lhs) array_expr_lhs = lhs; @@ -2372,9 +2366,6 @@ is_builtin_array_notation_fn (tree func_name, an_reduce_type *type) function_name = IDENTIFIER_POINTER (DECL_NAME (func_name)); } - if (!function_name) - return false; - if (!strcmp (function_name, "__sec_reduce_add")) { *type = REDUCE_ADD; @@ -2444,7 +2435,9 @@ is_builtin_array_notation_fn (tree func_name, an_reduce_type *type) } -/* Returns true if EXPR (and its subtrees) contain ARRAY_NOTATION_EXPR node. */ +/* Returns true if EXPR (and its subtrees) contain ARRAY_NOTATION_EXPR node. + If EXPR is NULL_TREE or does not contain any array notations, then the + function returns false. */ bool contains_array_notation_expr (tree expr) @@ -2780,13 +2773,12 @@ fix_array_notation_call_expr (tree arg) /* Walks through tree node T and find all the call-statments that do not return anything and fix up any array notations they may carry. The return value is the same type as T but with all array notations replaced with appropriate - STATEMENT_LISTS. Since this function recursively steps through all the - subtrees, t can be a NULL_TREE. If so, then the function just returns. */ + STATEMENT_LISTS. */ tree expand_array_notation_exprs (tree t) { - if (!t || !contains_array_notation_expr (t)) + if (!contains_array_notation_expr (t)) return t; switch (TREE_CODE (t)) @@ -2801,8 +2793,12 @@ expand_array_notation_exprs (tree t) subtrees. */ if (TREE_CODE (t) == COND_EXPR) { - COND_EXPR_THEN (t) = expand_array_notation_exprs (COND_EXPR_THEN (t)); - COND_EXPR_ELSE (t) = expand_array_notation_exprs (COND_EXPR_ELSE (t)); + if (COND_EXPR_THEN (t)) + COND_EXPR_THEN (t) = + expand_array_notation_exprs (COND_EXPR_THEN (t)); + if (COND_EXPR_ELSE (t)) + COND_EXPR_ELSE (t) = + expand_array_notation_exprs (COND_EXPR_ELSE (t)); } else t = expand_array_notation_exprs (t);