Re: [PATCH] Cilk Plus Array Notation for C++

[Jason Merrill <jason at redhat dot com>]

A few more comments:

> +         if (processing_template_decl || !TREE_TYPE (t))
> +           new_var = build_min_nt_loc (EXPR_LOCATION (t), VAR_DECL, NULL_TREE,
> +                                       NULL_TREE);

Again, we shouldn't be trying to expand array notation during template 
parsing.

And replace_invariant_exprs should also use get_temp_regvar, as should 
most of the places you create temporary variables.

> +      /* We need to do this because we are "faking" the builtin function types,
> +        so the compiler does a bunch of typecasts and this will get rid of
> +        all that!  */

Again, magic_varargs_p can avoid any gratuitous type conversions.

> +    case BUILT_IN_CILKPLUS_SEC_REDUCE_ALL_ZERO:
> +    case BUILT_IN_CILKPLUS_SEC_REDUCE_ANY_ZERO:
> +    case BUILT_IN_CILKPLUS_SEC_REDUCE_ANY_NONZERO:
> +    case BUILT_IN_CILKPLUS_SEC_REDUCE_ALL_NONZERO:
> +      new_var_type = integer_type_node;

I would expect boolean_type_node.

> +      an_loop_info[ii].var = build_decl (location, VAR_DECL, NULL_TREE,
> +                                 TREE_TYPE (an_info[0][ii].start));

Here you can use create_temporary_var.

> +      an_loop_info[ii].ind_init = build_x_modify_expr
> +       (location, an_loop_info[ii].var, NOP_EXPR,
> +        build_zero_cst (TREE_TYPE (an_loop_info[ii].var)),
> +        tf_warning_or_error);

Here and in other calls to build_x_modify_expr for initialization, use 
INIT_EXPR rather than NOP_EXPR.

> +       *new_var = create_tmp_var (new_var_type, NULL);

create_tmp_var is part of gimplification; it might be appropriate to use 
it when you move lowering to happen later in the compilation, but it 
seems wrong in the current code.

> +      code = (an_type == BUILT_IN_CILKPLUS_SEC_REDUCE_ANY_ZERO) ? EQ_EXPR
> +       : NE_EXPR;

A ?: expression split across multiple lines should have parens around it.

> +  /* If both are scalar, then the only reason why we will get this far is if
> +     there is some array notations inside it and was using a builtin array
> +     notation functions.  If so, we have already broken those guys up and now
> +     a simple build_x_modify_expr would do.  */
> +  if (lhs_rank == 0 && rhs_rank == 0)
> +    {
> +      if (found_builtin_fn)
> +       {
> +         new_modify_expr = build_x_modify_expr (location, lhs,
> +                                                modifycode, rhs, complain);
> +         finish_expr_stmt (new_modify_expr);
> +         pop_stmt_list (an_init);
> +         return an_init;
> +       }
> +      else
> +       {
> +         pop_stmt_list (an_init);
> +         return NULL_TREE;
> +       }
> +    }

The comment makes it sound like the else should be gcc_unreachable.

> +      if (location == UNKNOWN_LOCATION && EXPR_HAS_LOCATION (rhs))
> +       location = EXPR_LOCATION (rhs);

This is redundant with code a few lines above.

> +         append_to_statement_list_force (lhs_an_loop_info[ii].ind_init,
> +                                         &init_list);

Why do you need _force?

Jason