> -----Original Message-----
> From: Jason Merrill [mailto:[email protected]]
> Sent: Tuesday, June 25, 2013 10:39 AM
> To: Iyer, Balaji V; Richard Henderson
> Cc: Aldy Hernandez; [email protected]
> 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
