> -----Original Message----- > From: Jason Merrill [mailto:ja...@redhat.com] > Sent: Tuesday, June 25, 2013 10:39 AM > To: Iyer, Balaji V; Richard Henderson > Cc: Aldy Hernandez; gcc-patches@gcc.gnu.org > Subject: Re: [PATCH] Cilk Plus Array Notation for C++ > > On 06/24/2013 06:23 PM, Iyer, Balaji V wrote: > >> 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 I do it this way, then I don't think I will be able to handle a normal > > array > reference when cilk plus is enabled. > > What I had in mind is that in the case of a normal array reference, > cp_parser_array_notation will update index (which is passed by address) and > return NULL_TREE, so that it gets back on the normal path. > > > One thing I could do is to assume that people won't use array notations in > braced list. I had it like this a while back but I made the change to make > sure I > have covered more cases. Please let me know if that is OK and I will fix it. > > Yes, that's OK. >
> > I looked into this. But, magic_varargs_p is static to call.c. Should I make > > it non- > static? > > Yes. > > > After making it non-static I was planning to do something like this: > > > > if (magic_varargs_p (fndecl) > > Nargs = (*params)->length (); > > else > > nargs = convert_arguments (...) > > > > Is that OK with you? > > I was thinking to check magic_varargs_p in convert_arguments, where it > currently has > > > if (fndecl && DECL_BUILT_IN (fndecl) > > && DECL_FUNCTION_CODE (fndecl) == BUILT_IN_CONSTANT_P) > > /* Don't do ellipsis conversion for __built_in_constant_p > > as this will result in spurious errors for non-trivial > > types. */ > > change to "if (fndecl && magic_varargs_p (fndecl))" > This change is implemented. > >> 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? > > > > I use the different parts of array notations for error checking and to > > create the > outer for-loop. Also, to create the ARRAY_REF I need the induction variable. > > I understand the need for cilkplus_an_loop_parts, but cilkplus_an_parts seems > to contain exactly the same information as the ARRAY_NOTATION_REF. > > > Fixed! By the way, what is SFINAE? > > SFINAE stands for "substitution failure is not an error". During template > argument deduction, once we have a full set of template arguments we try to > substitute them into the template declaration. In that context, things that > would normally cause an error just fail and return error_mark_node silently. > > > diff --git a/gcc/cp/ChangeLog b/gcc/cp/ChangeLog index > > 55ed6a5..9d570b2 100644 Binary files a/gcc/cp/ChangeLog and > > b/gcc/cp/ChangeLog differ > > This patch still has ChangeLog entries. > This time, I ran the command you gave me. Please tell me how it looks. > > + new_var = get_temp_regvar (TREE_TYPE (retval_expr), > > + build_zero_cst (TREE_TYPE (retval_expr))); > > new_mod = expand_an_in_modify_expr (loc, new_var, NOP_EXPR, > > + TREE_OPERAND (retval_expr, 1), > > + tf_warning_or_error); > > Why not pass TREE_OPERAND (retval_expr, 1) as the init to get_temp_regvar? > new_var = TREE_OPERAND (retval_expr, 1) and if TREE_OPERAND (retval_expr, 1) has array notations then it wont get expanded correctly. Another solution is to replace get_tmp_regvar with get_temporary_var () + add_decl_expr (..). I have implemented this because it looks "more correct" > > - parser->colon_corrects_to_scope_p = > saved_colon_corrects_to_scope_p; > > if (!length_index || length_index == error_mark_node) > > cp_parser_skip_to_end_of_statement (parser); > > > > @@ -6180,7 +6158,6 @@ cp_parser_array_notation (location_t loc, > cp_parser *parser, tree init_index, > > saved_colon_corrects_to_scope_p = > > parser->colon_corrects_to_scope_p; > > /* Disable correcting single colon correcting to scope. */ > > - parser->colon_corrects_to_scope_p = false; > > This change looks like now you will only restore > parser->colon_corrects_to_scope_p if you have a stride. I would suggest > putting back the first restore, and removing all the corrects_to_scope_p code > from the stride block, since there can't be an array-notation colon after the > stride. I am setting the scope correction to false right before I look for length and restore it right after I parse the scope (i.e. outside the if-statement). I think this should fix the issue. > > >>> + /* 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'. > > > > As you suggested further in the email, I have made var of type > > ptrdiff_type, so > I think I should keep this. > > I think it would be better to convert start/stride to ptrdiff_t. > I don't think I can do that. Stride can be negative and if I am not mistaken, ptrdiff_t is unsigned. > Incidentally, types should be compared with same_type_p rather than ==. > > > + if (lhs_list_size > 0 && rhs_list_size > 0 && lhs_rank > 0 && rhs_rank > > > 0 > > + && TREE_CODE (lhs_len) == INTEGER_CST && rhs_len > > + && TREE_CODE (rhs_len) == INTEGER_CST) > > + { > > + HOST_WIDE_INT l_length = int_cst_value (lhs_len); > > + HOST_WIDE_INT r_length = int_cst_value (rhs_len); > > + if (absu_hwi (l_length) != absu_hwi (r_length)) > > + { > > + error_at (location, "length mismatch between LHS and RHS"); > > + pop_stmt_list (an_init); > > + return error_mark_node; > > + } > > + } > > Another place that should use tree_int_cst_equal. > Fixed! > > + /* No need to compare ii < rhs_rank && ii >= lhs_rank because valid > > Array > > + notation expression cannot RHS's rank cannot be greater than > > + LHS. */ > > Too many "cannot"s. > Sorry. Fixed. > > + if (processing_template_decl) > > + { > > + array_type = TREE_TYPE (array_value); > > + type = TREE_TYPE (array_type); > > + } > > We should be able to parse array notation in a template even when the array > expression has unknown type. In a template, just parse and remember the raw > expressions without worrying about diagnostics and conversions. > > > + if (flag_enable_cilkplus && contains_array_notation_expr (expr)) > > + { > > + size_t rank = 0; > > + > > + if (!find_rank (input_location, expr, expr, false, &rank)) > > + return error_mark_node; > > + > > + /* If the return expression contains array notations, then flag it as > > + error. */ > > + if (rank >= 1) > > + { > > + error_at (input_location, "array notation expression cannot be " > > + "used as a return value"); > > + return error_mark_node; > > + } > > + } > ... Fixed! > > + /* If an array's index is an array notation, then its rank cannot be > > + greater than one. */ > > + if (flag_enable_cilkplus && contains_array_notation_expr (idx)) > > + { > > + size_t rank = 0; > > + > > + /* If find_rank returns false, then it should have reported an error, > > + thus it is unnecessary for repetition. */ > > + if (!find_rank (loc, idx, idx, true, &rank)) > > + return error_mark_node; > > + if (rank > 1) > > + { > > + error_at (loc, "rank of the array%'s index is greater than 1"); > > + return error_mark_node; > > + } > > + } This one error is much easier to do it here than anywhere else. An array_ref could be a parameter inside a function, part of a modify expression, unary expression etc. If I move it to transformation stage, I have to do checks in all these places and there is a small chance some will slip through the cracks. This is almost a fool proof way of doing it. Such things have been done before. For example, Open MP does a return expression check in finish_return_stmt (even though this is a different issue we are talking about). If it is the code lines that is an issue, then I am willing to enclose that in a function or #define. > > + if (flag_enable_cilkplus && contains_array_notation_expr (op0)) > > + type0 = find_correct_array_notation_type (op0); else > > + type0 = TREE_TYPE (op0); > ... Fixed! > > + if (flag_enable_cilkplus && TREE_CODE (arg) == ARRAY_NOTATION_REF) > > + { > > + val = build_address (arg); > > + if (TREE_CODE (arg) == OFFSET_REF) > > + PTRMEM_OK_P (val) = PTRMEM_OK_P (arg); > > + return val; > > + } Fixed! > ... > > + /* If we are dealing with built-in array notation function then we don't > > need > > + to convert them. They will be broken up into modify exprs in future, > > + during which all these checks will be done. */ if > > + (flag_enable_cilkplus > > + && is_cilkplus_reduce_builtin (fndecl) != BUILT_IN_NONE) > > + return rhs; > > More changes that should be unnecessary since ARRAY_NOTATION_REF has a > normal type. > Fixed! > What remaining obstacles are there to sharing most of the expansion code > between C and C++? That can be a separate patch, of course. > > Jason