From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 1580 invoked by alias); 22 Jun 2013 16:20:45 -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 1556 invoked by uid 89); 22 Jun 2013 16:20:39 -0000 X-Spam-SWARE-Status: No, score=-7.7 required=5.0 tests=AWL,BAYES_00,KHOP_THREADED,RCVD_IN_HOSTKARMA_W,RCVD_IN_HOSTKARMA_WL,RP_MATCHES_RCVD,SPF_HELO_PASS,SPF_PASS,TW_TM autolearn=ham version=3.3.1 Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.84/v0.84-167-ge50287c) with ESMTP; Sat, 22 Jun 2013 16:20:29 +0000 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r5MGKR7J010958 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Sat, 22 Jun 2013 12:20:27 -0400 Received: from [10.3.113.51] (ovpn-113-51.phx2.redhat.com [10.3.113.51]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id r5MGKPuk003854; Sat, 22 Jun 2013 12:20:26 -0400 Message-ID: <51C5CEC9.8050309@redhat.com> Date: Sat, 22 Jun 2013 16:20:00 -0000 From: Jason Merrill User-Agent: Mozilla/5.0 (X11; Linux i686; rv:23.0) Gecko/20100101 Thunderbird/23.0a2 MIME-Version: 1.0 To: "Iyer, Balaji V" , Richard Henderson CC: Aldy Hernandez , "gcc-patches@gcc.gnu.org" Subject: Re: [PATCH] Cilk Plus Array Notation for C++ References: <51B8A2FA.2020404@redhat.com> <51B9EF1D.9060505@redhat.com> <51B9F0EA.50709@redhat.com> <51BF4222.3050107@redhat.com> <51C0F06E.3080507@redhat.com> <51C341D2.8020707@redhat.com> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit X-SW-Source: 2013-06/txt/msg01323.txt.bz2 Hmm, seems like I should have sent this yesterday even though I hadn't made it through the whole patch. But I suppose it doesn't hurt to fix it after checkin. On 06/20/2013 07:39 PM, Iyer, Balaji V wrote: > diff --git a/gcc/c-family/ChangeLog b/gcc/c-family/ChangeLog > old mode 100644 > new mode 100755 > index a0f195b..fb70520 > Binary files a/gcc/c-family/ChangeLog and b/gcc/c-family/ChangeLog differ Why are you marking lots of files as executable? In the future please filter out ChangeLogs entirely from diffs. I use filterdiff -x '*/ChangeLog' | sed -e '/^diff.*ChangeLog/{N;d}' for my own patches. > + if (flag_enable_cilkplus > + && (contains_array_notation_expr (expr) > + || contains_array_notation_expr (fn))) Looking at "fn" here doesn't make sense; it's only used for diagnostics. > + /* If we are using array notations, we fix them up at a later stage > + and we will do these checks then. */ > + ; And please don't mess with the overload resolution code directly to handle this case differently. This seems to be a general problem with the patch; you are special-casing array notation all through the compiler rather than making it work with the existing mechanisms. In this case, an ARRAY_NOTATION_REF should have a type that participates normally with conversions. >> + /* If the function call is builtin array notation function then no need >> + to do any type conversion. */ And here, instead of changing build_over_call, you can use the existing magic_varargs_p mechanism. > +/* This function parses Cilk Plus array notations. The starting index is > + passed in INIT_INDEX and the array name is passed in ARRAY_VALUE. If the > + INIT_INDEX is NULL, then we have special case were the entire array is "where" > + accessed (e.g. A[:]). The return value of this function is a tree node > + called VALUE_TREE of type ARRAY_NOTATION_REF. If some error occurred it Drop "called VALUE_TREE"; the function comment shouldn't talk about local variables. > + cp_token *token = NULL; > + tree start_index = NULL_TREE, length_index = NULL_TREE, stride = NULL_TREE; > + tree value_tree, type, array_type, array_type_domain; > + double_int x; > + bool saved_colon_corrects_to_scope_p = parser->colon_corrects_to_scope_p; Now that the compiler is built as C++, variables should be defined at the point of first use, rather than at the top of the function. > + if (TREE_CODE (array_type) == RECORD_TYPE > + || TREE_CODE (array_type) == POINTER_TYPE) > + { > + error_at (loc, "start-index and length fields necessary for " > + "using array notations in pointers or records"); I think this should handle all non-array types, rather than just pointers and classes. Let's say "notation" rather than "notations" in all diagnostics. > + error_at (loc, "array notations cannot be used with" > + " function pointer arrays"); I don't see this restriction in any of the documentation. What's the rationale? > + error_at (loc, "start-index and length fields necessary for " > + "using array notations in dimensionless arrays"); Let's say "...with array of unknown bound" > + start_index = fold_build1 (CONVERT_EXPR, ptrdiff_type_node, > + start_index); Use cp_fold_convert rather than fold_build1. > + x = TREE_INT_CST (TYPE_MAXVAL (array_type_domain)); > + x.low++; > + length_index = double_int_to_tree (integer_type_node, x); This assumes a constant length array. Use size_binop instead. > + stride = build_int_cst (integer_type_node, 1); > + stride = fold_build1 (CONVERT_EXPR, ptrdiff_type_node, stride); Build the constant in ptrdiff_type_node rather than build it in integer_type_node and then convert. > + /* Disable correcting single colon correcting to scope. */ > + parser->colon_corrects_to_scope_p = false; > + stride = cp_parser_expression (parser, false, NULL); > + parser->colon_corrects_to_scope_p = > + saved_colon_corrects_to_scope_p; Why do you do this for the stride? We've already seen both array-notation colons at this point. > + /* We fold all 3 of the values to make things easier when we transform > + them later. */ Why is this better than folding at transformation time? > + /* If we are here, then we have something like this: > + ARRAY[:] > + */ The */ should go at the end of the last line in all comments. > - if (for_offsetof) > - index = cp_parser_constant_expression (parser, false, NULL); ... > else > { > - if (cp_lexer_next_token_is (parser->lexer, CPP_OPEN_BRACE)) ... > + if (for_offsetof) > + index = cp_parser_constant_expression (parser, false, NULL); > + else Please try to avoid re-indenting code when possible, to make history annotation simpler. Actually, to reduce the amount of changes to non-AN code, let's put the AN case third, after the offset and {} cases, so you get something like else if (flag_enable_cilkplus) { tree an = cp_parser_array_notation (loc, parser, &index, postfix_expression); if (an) return an; /* Otherwise, cp_parser_array_notation set 'index'. */ } else index = cp_parser_expression (parser, /*cast_p=*/false, NULL); this way the change is just a few added lines, and everything else is in cp_parser_array_notation. > + if (flag_enable_cilkplus && contains_array_notation_expr (compound_stmt)) > + compound_stmt = expand_array_notation_exprs (compound_stmt); ... > + /* Transform all array notations to the equivalent array refs and loop. */ > + if (flag_enable_cilkplus && contains_array_notation_expr (body)) > + body = expand_array_notation_exprs (body); ... > + /* Expand all array notation expressions here. */ > + if (flag_enable_cilkplus && current_function_decl > + && contains_array_notation_expr (DECL_SAVED_TREE (current_function_decl))) > + DECL_SAVED_TREE (current_function_decl) = > + expand_array_notation_exprs (DECL_SAVED_TREE (current_function_decl)); Let's do the transformation in cp_genericize rather than in these places. Perhaps moving the expansion to gimplification time would also help with sharing code between C and C++, since you don't have to worry about templates at that point. > + if (flag_enable_cilkplus > + && contains_array_notation_expr (condition)) > + { > + error_at (EXPR_LOCATION (condition), > + "array notations cannot be used as a condition for " > + "switch statement"); > + statement = error_mark_node; > + } [etc] I'd prefer to give diagnostics about uses that don't make sense at transformation time, rather than parsing time. > + if (flag_enable_cilkplus > + && cp_lexer_next_token_is (parser->lexer, CPP_COLON)) > + { > + bounds = error_mark_node; > + error_at (cp_lexer_peek_token (parser->lexer)->location, > + "array notations cannot be used in declaration"); > + cp_lexer_consume_token (parser->lexer); > + } I'm concerned about this causing trouble with tentative parsing, and I think that people are unlikely to try to write a declaration using array notation, so let's leave out the changes to cp_parser_direct_declarator. > @@ -15758,6 +15772,9 @@ type_unification_real (tree tparms, > + if (flag_enable_cilkplus && TREE_CODE (arg) == ARRAY_NOTATION_REF) > + return 1; Again, an ARRAY_NOTATION_REF should have a type like any other expression; we don't need to prevent it from participating in type deduction. > @@ -8073,6 +8089,7 @@ cxx_eval_constant_expression (const constexpr_call *call, tree t, > + case ARRAY_NOTATION_REF: > @@ -8884,6 +8901,7 @@ potential_constant_expression_1 (tree t, bool want_rval, tsubst_flags_t flags) > + case ARRAY_NOTATION_REF: cxx_eval_array_reference doesn't know how to deal with ARRAY_NOTATION_REF, so let's not add it to cxx_eval_constant_expression, and return false for it from potential_constant_expression_1. > + if (flag_enable_cilkplus > + && is_cilkplus_reduce_builtin (fndecl) != BUILT_IN_NONE) > + nargs = (*params)->length (); > + else > + nargs = convert_arguments (parm_types, params, fndecl, LOOKUP_NORMAL, > + complain); Another change that should be replaced by addition to magic_varargs_p, though it looks like convert_arguments needs to be updated to use magic_varargs_p rather than checking specifically for BUILT_IN_CONSTANT_P. > + for (size_t ii = 0; ii < size; ii++) > + for (size_t jj = 0; jj < rank; jj++) > + { > + (*node)[ii].safe_push (init); > + array_exprs[ii].safe_push (NULL_TREE); > + } Why not use safe_grow_cleared for the sub-vecs as well? > + while (ii_tree) > + if (TREE_CODE (ii_tree) == ARRAY_NOTATION_REF) Wrap the sub-statement of the 'while' in { }. > + else if (TREE_CODE (ii_tree) == VAR_DECL > + || TREE_CODE (ii_tree) == CALL_EXPR > + || TREE_CODE (ii_tree) == PARM_DECL) > + break; > + else > + gcc_unreachable (); There are lots of other ways to get an expression of array type, why are only these allowed? > + if (TREE_CODE ((*list)[ii]) == ARRAY_NOTATION_REF) > + for (size_t jj = 0; jj < rank; jj++) > + if (TREE_CODE (array_exprs[ii][jj]) == ARRAY_NOTATION_REF) How could the array_exprs element be anything other than an ARRAY_NOTATION_REF? That's all you put in when you were building it. By the way, why are you breaking out the elements of the ARRAY_NOTATION_REF into a cilkplus_an_parts rather than using the _REF directly? > + if (TREE_CODE ((*list)[ii]) == CALL_EXPR > + && TREE_CODE (CALL_EXPR_FN ((*list)[ii])) == ADDR_EXPR > + && is_sec_implicit_index_fn (CALL_EXPR_FN ((*list)[ii]))) Why check for ADDR_EXPR here? Better for is_sec_implicit_index_fn to return false if its argument isn't what's desired. > + if (idx < (int) rank && idx >= 0) > + vec_safe_push (array_operand, an_loop_info[idx].var); > + else if (idx == -1) > + /* In this case, the returning function would have emitted an > + error thus it is not necessary to do so again. */ > + return NULL; Reorder these cases so you don't need to check idx >= 0. > + same dimension and one the same side of the equal sign. The Array notation "on the same side" > + l_node = int_cst_value (list[ii][jj].length); > + l_length = int_cst_value (length); > + if (absu_hwi (l_length) != absu_hwi (l_node)) Use tree_int_cst_equal instead. > + for (jj = 0; jj < y; jj++) > + { > + length = NULL_TREE; > + for (ii = 0; ii < x; ii++) Why did you switch the outer/inner iteration variables for this loop compared to others? > + /* We set the length node as the current node just in case it turns > + out to be an integer. */ > + length = list[ii][jj].length; How could it "turn out" to be an integer? We just checked, and it isn't one. > + var = build_decl (loc, VAR_DECL, NULL_TREE, integer_type_node); > + finish_expr_stmt (build_x_modify_expr (loc, var, NOP_EXPR, *value, cry)); This should use get_temp_regvar. We shouldn't need to pass tsubst_flags_t around, since we will never be expanding array notation in SFINAE context. > + /* If stride and start are of same type and the induction var > + is not, convert induction variable to stride's type. */ > + if (TREE_TYPE (start) == TREE_TYPE (stride) > + && TREE_TYPE (stride) != TREE_TYPE (var)) This seems impossible, since 'var' is created to have the same type as 'start'. > + /* If we reach here, then the stride and start are of > + different types, and so it doesn't really matter what > + the induction variable type is, convert everything to > + integer. The reason why we pick an integer > + instead of something like size_t is because the stride > + and length can be + or -. */ Maybe the induction variable should always be ptrdiff_type_node. Jason