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
