It looks like a NULL in INIT_INDEX is a specially handled case. Perhaps you
should document that INIT_INDEX can be null and what it means.
Also, you don't need to document what internal variable name you are using as
a return value (VALUE_TREE). Perhaps instead of "The return value..." you could
write "This function returns the ARRAY_NOTATION_REF node." or something
like it.
It is documented inside the function, right before checking for !init_index. Is
that enough?
Please put it in the function comment. As it stands, users would have
to look through the body to find that init_index==NULL is a special case.
Changes to existing tests should be submitted as a separate patch, since this
doesn't seem to be C++ specific. And BTW, this particular test change can be
committed as obvious.
OK will do. What about adding {target c} and {target c++} for test cases. Can
that be submitted with the patch?
Yes.
+ else if (an_type == BUILT_IN_CILKPLUS_SEC_REDUCE_MAX)
+ {
+ /* If the TYPE_MIN_VALUE is available for the new_var_type, then
+ set that as the initial value. */
+ if (TYPE_MIN_VALUE (new_var_type))
+ new_var_init = build_x_modify_expr (location, *new_var, NOP_EXPR,
+ TYPE_MIN_VALUE (new_var_type), 1);
+ else
+ /* ... otherwise set initial value as the first element of array. */
+ new_var_init = build_x_modify_expr (location, *new_var, NOP_EXPR,
+ func_parm, 1);
+ new_no_expr = build_x_modify_expr (location, *new_var, NOP_EXPR,
+ *new_var, 1);
+ new_yes_expr = build_x_modify_expr (location, *new_var, NOP_EXPR,
+ func_parm, 1);
+ new_cond_expr = build_x_binary_op (location, LT_EXPR, *new_var,
+ TREE_CODE (*new_var), func_parm,
+ TREE_CODE (func_parm), NULL,
+ tf_warning_or_error);
+ new_expr = build_x_conditional_expr (location, new_cond_expr,
+ new_yes_expr, new_no_expr,
+ tf_warning_or_error);
+ }
+ else if (an_type == BUILT_IN_CILKPLUS_SEC_REDUCE_MIN)
+ {
+ /* If the TYPE_MAX_VALUE is available for the new_var_type, then
+ set that as the initial value. */
+ if (TYPE_MAX_VALUE (new_var_type))
+ new_var_init = build_x_modify_expr (location, *new_var, NOP_EXPR,
+ TYPE_MAX_VALUE (new_var_type), 1);
+ else
+ /* ... otherwise set initial value as the first element of array. */
+ new_var_init = build_x_modify_expr (location, *new_var, NOP_EXPR,
+ func_parm, 1);
+ new_no_expr = build_x_modify_expr (location, *new_var, NOP_EXPR,
+ *new_var, 1);
+ new_yes_expr = build_x_modify_expr (location, *new_var, NOP_EXPR,
+ func_parm, 1);
+ new_cond_expr = build_x_binary_op (location, GT_EXPR, *new_var,
+ TREE_CODE (*new_var), func_parm,
+ TREE_CODE (func_parm), NULL,
+ tf_warning_or_error);
+ new_expr = build_x_conditional_expr (location, new_cond_expr,
+ new_yes_expr, new_no_expr,
+ tf_warning_or_error);
+ }
The whole slew of these cases have a lot of duplicated code. For
instance, BUILT_IN_CILKPLUS_SEC_REDUCE_MIN is the same as
BUILT_IN_CILKPLUS_SEC_REDUCE_MAX, the only difference being GT_EXPR vs
LT_EXPR. Surely you could do something like:
if (an_type == BUILT_IN_CILKPLUS_SEC_REDUCE_MIN
|| an_type == BUILT_IN_CILKPLUS_SEC_REDUCE_MAX) {
enum tree_code code;
if (an_type == BUILT_IN_CILKPLUS_SEC_REDUCE_MIN)
code = GT_EXPR;
else
code = LT_EXPR;
// stuff
}
The compiler should be able to optimize the above, but even if it
couldn't, I am willing to compare twice and save lines and lines of code.
Similarly for SEC_REDUCE_ANY_ZERO/SEC_REDUCE_ANY_NONZERO,
SEC_REDUCE_ALL_ZERO/SEC_REDUCE_ALL_NONZERO,
SEC_REDUCE_ADD/SEC_REDUCE_MUL, etc etc etc.
+ if (location == UNKNOWN_LOCATION)
+ {
+ if (EXPR_LOCATION (lhs) != UNKNOWN_LOCATION)
+ location = EXPR_LOCATION (lhs);
+ else if (EXPR_LOCATION (rhs) != UNKNOWN_LOCATION)
+ location = EXPR_LOCATION (rhs);
+ }
+
+
+ /* We need this when we have a scatter issue. */
Extra whitespace.
+ if (lhs_rank != 0 && rhs_rank != 0 && lhs_rank != rhs_rank)
+ {
+ tree lhs_base = lhs;
+ tree rhs_base = rhs;
+
+ for (ii = 0; ii < lhs_rank; ii++)
+ lhs_base = ARRAY_NOTATION_ARRAY (lhs_base);
+
+ while (rhs_base && TREE_CODE (rhs_base) != ARRAY_NOTATION_REF)
+ rhs_base = TREE_OPERAND (rhs_base, 0);
+ for (ii = 0; ii < rhs_rank; ii++)
+ rhs_base = ARRAY_NOTATION_ARRAY (rhs_base);
+
+ if (location == UNKNOWN_LOCATION && EXPR_HAS_LOCATION (lhs))
+ location = EXPR_LOCATION (lhs);
+ error_at (location, "rank mismatch between %qD and %qD", lhs_base,
+ rhs_base);
+ return error_mark_node;
+ }
This whole block seems to be indented incorrectly.
+ /* Assign the array notation components to variable so that they can satisfy
+ the exec-once rule. */
+ for (ii = 0; ii < lhs_list_size; ii++)
+ {
+ tree array_node = (*lhs_list)[ii];
+ tree array_begin = ARRAY_NOTATION_START (array_node);
+ tree array_lngth = ARRAY_NOTATION_LENGTH (array_node);
+ tree array_strde = ARRAY_NOTATION_STRIDE (array_node);
+
+ if (TREE_CODE (array_begin) != INTEGER_CST)
+ {
+ lhs_begin_var = build_decl (location, VAR_DECL, NULL_TREE,
+ integer_type_node);
+ finish_expr_stmt (build_x_modify_expr (location, lhs_begin_var,
+ NOP_EXPR, array_begin,
+ complain));
+ ARRAY_NOTATION_START (array_node) = lhs_begin_var;
+ }
+ if (TREE_CODE (array_lngth) != INTEGER_CST)
+ {
+ lhs_lngth_var = build_decl (location, VAR_DECL, NULL_TREE,
+ integer_type_node);
+ finish_expr_stmt (build_x_modify_expr (location, lhs_lngth_var,
+ NOP_EXPR, array_lngth,
+ complain));
+ ARRAY_NOTATION_LENGTH (array_node) = lhs_lngth_var;
+ }
All these exec-one wrappers seem to be doing the same thing over and
over. Can you abstract this sequence into a separate helper function?
Perhaps something like:
for (ii = 0; ii < lhs_list_size; ii++)
{
exec_once (&ARRAY_NOTATION_START (array_node));
exec_once (&ARRAY_NOTATION_LENGTH (array_node));
exec_once (&ARRAY_NOTATION_STRIDE (array_node));
...
...
}
or something to that effect.
+ if (lhs_rank)
+ {
+ for (ii = 0; ii < lhs_list_size; ii++)
+ {
+ jj = 0;
+ ii_tree = (*lhs_list)[ii];
+ while (ii_tree)
+ {
+ if (TREE_CODE (ii_tree) == ARRAY_NOTATION_REF)
+ {
+ lhs_array[ii][jj] = ii_tree;
+ jj++;
+ ii_tree = ARRAY_NOTATION_ARRAY (ii_tree);
+ }
+ else if (TREE_CODE (ii_tree) == ARRAY_REF)
+ ii_tree = TREE_OPERAND (ii_tree, 0);
+ else if (TREE_CODE (ii_tree) == VAR_DECL
+ || TREE_CODE (ii_tree) == PARM_DECL)
+ break;
+ }
+ }
+ }
+ else
+ lhs_array[0][0] = NULL_TREE;
+
+ if (rhs_rank)
+ {
+ for (ii = 0; ii < rhs_list_size; ii++)
+ {
+ jj = 0;
+ ii_tree = (*rhs_list)[ii];
+ while (ii_tree)
+ {
+ if (TREE_CODE (ii_tree) == ARRAY_NOTATION_REF)
+ {
+ rhs_array[ii][jj] = ii_tree;
+ jj++;
+ ii_tree = ARRAY_NOTATION_ARRAY (ii_tree);
+ }
+ else if (TREE_CODE (ii_tree) == ARRAY_REF)
+ ii_tree = TREE_OPERAND (ii_tree, 0);
+ else if (TREE_CODE (ii_tree) == VAR_DECL
+ || TREE_CODE (ii_tree) == PARM_DECL
+ || TREE_CODE (ii_tree) == CALL_EXPR)
+ break;
+ }
+ }
+ }
Can't you abstract the common idiom into some inline function?
+ for (ii = 0; ii < lhs_list_size; ii++)
+ {
+ if (TREE_CODE ((*lhs_list)[ii]) == ARRAY_NOTATION_REF)
+ {
+ for (jj = 0; jj < lhs_rank; jj++)
+ {
+ if (TREE_CODE (lhs_array[ii][jj]) == ARRAY_NOTATION_REF)
+ {
+ lhs_value[ii][jj] = ARRAY_NOTATION_ARRAY (lhs_array[ii][jj]);
+ lhs_start[ii][jj] = ARRAY_NOTATION_START (lhs_array[ii][jj]);
+ lhs_length[ii][jj] =
+ fold_build1 (CONVERT_EXPR, integer_type_node,
+ ARRAY_NOTATION_LENGTH (lhs_array[ii][jj]));
+ lhs_stride[ii][jj] =
+ fold_build1 (CONVERT_EXPR, integer_type_node,
+ ARRAY_NOTATION_STRIDE (lhs_array[ii][jj]));
+ lhs_vector[ii][jj] = true;
+
+ /* If the stride value is variable (i.e. not constant) then
+ assume that the length is positive. */
+ if (!TREE_CONSTANT (lhs_length[ii][jj]))
+ lhs_count_down[ii][jj] = false;
+ else if (tree_int_cst_lt
+ (lhs_length[ii][jj],
+ build_zero_cst (TREE_TYPE (lhs_length[ii][jj]))))
+ lhs_count_down[ii][jj] = true;
+ else
+ lhs_count_down[ii][jj] = false;
+ }
+ else
+ lhs_vector[ii][jj] = false;
+ }
+ }
+ }
+ for (ii = 0; ii < rhs_list_size; ii++)
+ {
+ if (TREE_CODE ((*rhs_list)[ii]) == ARRAY_NOTATION_REF)
+ {
+ for (jj = 0; jj < rhs_rank; jj++)
+ {
+ if (TREE_CODE (rhs_array[ii][jj]) == ARRAY_NOTATION_REF)
+ {
+ rhs_value[ii][jj] = ARRAY_NOTATION_ARRAY (rhs_array[ii][jj]);
+ rhs_start[ii][jj] = ARRAY_NOTATION_START (rhs_array[ii][jj]);
+ rhs_length[ii][jj] =
+ ARRAY_NOTATION_LENGTH (rhs_array[ii][jj]);
+ rhs_stride[ii][jj] =
+ ARRAY_NOTATION_STRIDE (rhs_array[ii][jj]);
+ rhs_vector[ii][jj] = true;
+ /* If the stride value is variable (i.e. not constant) then
+ assume that the length is positive. */
+ if (!TREE_CONSTANT (rhs_length[ii][jj]))
+ rhs_count_down[ii][jj] = false;
+ else if (tree_int_cst_lt
+ (rhs_length[ii][jj],
+ build_zero_cst (TREE_TYPE (rhs_length[ii][jj]))))
+ rhs_count_down[ii][jj] = true;
+ else
+ rhs_count_down[ii][jj] = false;
+ }
+ else
+ rhs_vector[ii][jj] = false;
+ }
Similarly here, though I don't know if the CONVERT_EXPR in the first
case and not in the second case is an oversight.
+ if (lhs_rank)
+ {
+ for (ii = 0; ii < lhs_list_size; ii++)
+ {
+ if (lhs_vector[ii][0])
+ {
+ /* The last ARRAY_NOTATION element's ARRAY component should be
+ the array's base value. */
+ tree lhs_array_opr = lhs_value[ii][lhs_rank - 1];
+ for (s_jj = lhs_rank - 1; s_jj >= 0; s_jj--)
+ {
+ tree stride = NULL_TREE, var = NULL_TREE, start = NULL_TREE;
+ if ((TREE_TYPE (lhs_start[ii][s_jj]) ==
+ TREE_TYPE (lhs_stride[ii][s_jj]))
+ && (TREE_TYPE (lhs_stride[ii][s_jj]) !=
+ TREE_TYPE (lhs_var[s_jj])))
+ {
+ /* If stride and start are of same type and the induction
+ var is not, convert induction variable to stride's
+ type. */
+ start = lhs_start[ii][s_jj];
+ stride = lhs_stride[ii][s_jj];
+ var = build_c_cast (location,
+ TREE_TYPE (lhs_stride[ii][s_jj]),
+ lhs_var[s_jj]);
+ }
+ else if (TREE_TYPE (lhs_start[ii][s_jj]) !=
+ TREE_TYPE (lhs_stride[ii][s_jj]))
+ {
+ /* 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 -. */
+ start = build_c_cast (location, integer_type_node,
+ lhs_start[ii][s_jj]);
+ stride = build_c_cast (location, integer_type_node,
+ lhs_stride[ii][s_jj]);
+ var = build_c_cast (location, integer_type_node,
+ lhs_var[s_jj]);
+ }
+ else
+ {
+ start = lhs_start[ii][s_jj];
+ stride = lhs_stride[ii][s_jj];
+ var = lhs_var[s_jj];
+ }
+ if (lhs_count_down[ii][s_jj])
+ /* Array[start_index - (induction_var * stride)]. */
+ lhs_array_opr = grok_array_decl
+ (location, lhs_array_opr,
+ build2 (MINUS_EXPR, TREE_TYPE (var), start,
+ build2 (MULT_EXPR, TREE_TYPE (var), var,
+ stride)), false);
+ else
+ /* Array[start_index + (induction_var * stride)]. */
+ lhs_array_opr = grok_array_decl
+ (location, lhs_array_opr,
+ build2 (PLUS_EXPR, TREE_TYPE (var), start,
+ build2 (MULT_EXPR, TREE_TYPE (var), var,
+ stride)), false);
+ }
+ vec_safe_push (lhs_array_operand, lhs_array_opr);
+ }
+ else
+ vec_safe_push (lhs_array_operand, integer_one_node);
+ }
99% of this looks exactly like the rhs_rank case, surely you can
abstract most of this into a separate function.
+ for (ii = 0; ii < rhs_list_size; ii++)
+ {
+ tree rhs_node = (*rhs_list)[ii];
+ if (TREE_CODE (rhs_node) == CALL_EXPR)
+ {
+ int idx_value = 0;
+ tree func_name = CALL_EXPR_FN (rhs_node);
+ if (TREE_CODE (func_name) == ADDR_EXPR)
+ if (is_sec_implicit_index_fn (func_name))
+ {
+ idx_value =
+ extract_sec_implicit_index_arg (location, rhs_node);
+ if (idx_value < (int) lhs_rank && idx_value >= 0)
+ vec_safe_push (rhs_array_operand, rhs_var[idx_value]);
+ else
+ {
+ size_t ee = 0;
+ tree rhs_base = (*lhs_list)[ii];
+ for (ee = 0; ee < rhs_rank; ee++)
+ if (rhs_base
+ && TREE_CODE (rhs_base) == ARRAY_NOTATION_REF)
+ rhs_base = ARRAY_NOTATION_ARRAY (rhs_base);
+
+ error_at (location, "__sec_implicit_index argument %d "
+ "must be less than rank of %qD", idx_value,
+ rhs_base);
+ return error_mark_node;
+ }
+ }
+ }
+ }
+ replace_array_notations (&rhs, true, rhs_list, rhs_array_operand);
+ array_expr_rhs = rhs;
+ }
+ else
+ {
+ for (ii = 0; ii < rhs_list_size; ii++)
+ {
+ tree rhs_node = (*rhs_list)[ii];
+ if (TREE_CODE (rhs_node) == CALL_EXPR)
+ {
+ int idx_value = 0;
+ tree func_name = CALL_EXPR_FN (rhs_node);
+ if (is_sec_implicit_index_fn (func_name))
+ {
+ idx_value = extract_sec_implicit_index_arg (location,
+ rhs_node);
+ if (idx_value < (int) lhs_rank && idx_value >= 0)
+ vec_safe_push (rhs_array_operand, lhs_var[idx_value]);
+ else
+ {
+ size_t ee = 0;
+ tree lhs_base = (*lhs_list)[ii];
+ for (ee = 0; ee < lhs_rank; ee++)
+ if (lhs_base
+ && TREE_CODE (lhs_base) == ARRAY_NOTATION_REF)
+ lhs_base = ARRAY_NOTATION_ARRAY (lhs_base);
+ error_at (location, "__sec_implicit_index argument %d "
+ "must be less than the rank of %qD", idx_value,
+ lhs_base);
+ return error_mark_node;
+ }
+ }
+ }
Again, a lot of stuff that looks exactly the same.
Similarly in the rest of expand_an_in_modify_expr(). The function is
HUGE, and there's a lot of code duplication going on. Try to abstract
the things you do more than once into their own helper functions. This
will cut down on the maintenance burden.
+ if (TREE_CODE (orig_stmt) == COND_EXPR)
+ {
+ size_t cond_rank = 0, yes_rank = 0, no_rank = 0;
+ tree yes_expr = COND_EXPR_THEN (orig_stmt);
+ tree no_expr = COND_EXPR_ELSE (orig_stmt);
+ tree cond = COND_EXPR_COND (orig_stmt);
+ if (!find_rank (EXPR_LOCATION (cond), cond, cond, true, &cond_rank)
+ || !find_rank (EXPR_LOCATION (yes_expr), yes_expr, yes_expr, true,
+ &yes_rank)
+ || find_rank (EXPR_LOCATION (no_expr), no_expr, no_expr, true,
+ &no_rank))
+ return error_mark_node;
+ if (cond_rank != 0 && cond_rank != yes_rank && yes_rank != 0)
+ {
+ error_at (EXPR_LOCATION (yes_expr), "rank mismatch with controlling"
+ " expression of parent if-statement");
+ return error_mark_node;
+ }
+ else if (cond_rank != 0 && cond_rank != no_rank && no_rank != 0)
+ {
+ error_at (EXPR_LOCATION (no_expr), "rank mismatch with controlling "
+ "expression of parent if-statement");
+ return error_mark_node;
+ }
+ }
+ else if (TREE_CODE (orig_stmt) == IF_STMT)
+ {
+ size_t cond_rank = 0, yes_rank = 0, no_rank = 0;
+ tree yes_expr = THEN_CLAUSE (orig_stmt);
+ tree no_expr = ELSE_CLAUSE (orig_stmt);
+ tree cond = IF_COND (orig_stmt);
+ if (!find_rank (EXPR_LOCATION (cond), cond, cond, true, &cond_rank)
+ || (yes_expr
+ && !find_rank (EXPR_LOCATION (yes_expr), yes_expr, yes_expr, true,
+ &yes_rank))
+ || (no_expr
+ && !find_rank (EXPR_LOCATION (no_expr), no_expr, no_expr, true,
+ &no_rank)))
+ return error_mark_node;
+ if (cond_rank != 0 && cond_rank != yes_rank && yes_rank != 0)
+ {
+ error_at (EXPR_LOCATION (yes_expr), "rank mismatch with controlling"
+ " expression of parent if-statement");
+ return error_mark_node;
+ }
+ else if (cond_rank != 0 && cond_rank != no_rank && no_rank != 0)
+ {
+ error_at (EXPR_LOCATION (no_expr), "rank mismatch with controlling "
+ "expression of parent if-statement");
+ return error_mark_node;
+ }
+ }
And here in cp_expand_cond_array_notations().
+ for (ii = 0; ii < list_size; ii++)
+ {
+ jj = 0;
+ jj_tree = (*array_list)[ii];
+ while (jj_tree)
+ {
+ if (TREE_CODE (jj_tree) == ARRAY_NOTATION_REF)
+ {
+ array_ops[ii][jj] = jj_tree;
+ jj++;
+ jj_tree = ARRAY_NOTATION_ARRAY (jj_tree);
+ }
+ else if (TREE_CODE (jj_tree) == ARRAY_REF)
+ jj_tree = TREE_OPERAND (jj_tree, 0);
+ else if (TREE_CODE (jj_tree) == VAR_DECL
+ || TREE_CODE (jj_tree) == PARM_DECL)
+ break;
+ }
+ }
The above snippet appears in the exact same form in both
expand_sec_reduce_builtin() and cp_expand_cond_array_notations(). I'm
not going to mention any more of these duplicated code sequences, but
you should go through the entire file and abstract what you can into
separate inlineable functions, no sense maintaining 10 copies of the
same thing.
Aldy
