This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH] Cilk Plus Array Notation for C++
- From: Jason Merrill <jason at redhat dot com>
- To: "Iyer, Balaji V" <balaji dot v dot iyer at intel dot com>, Richard Henderson <rth at redhat dot com>
- Cc: Aldy Hernandez <aldyh at redhat dot com>, "gcc-patches at gcc dot gnu dot org" <gcc-patches at gcc dot gnu dot org>
- Date: Mon, 24 Jun 2013 11:16:12 -0400
- Subject: Re: [PATCH] Cilk Plus Array Notation for C++
- References: <BF230D13CA30DD48930C31D4099330003A42D85F at FMSMSX101 dot amr dot corp dot intel dot com> <51B8A2FA dot 2020404 at redhat dot com> <BF230D13CA30DD48930C31D4099330003A42E4B8 at FMSMSX101 dot amr dot corp dot intel dot com> <51B9EF1D dot 9060505 at redhat dot com> <51B9F0EA dot 50709 at redhat dot com> <BF230D13CA30DD48930C31D4099330003A439163 at FMSMSX101 dot amr dot corp dot intel dot com> <51BF4222 dot 3050107 at redhat dot com> <BF230D13CA30DD48930C31D4099330003A4391EA at FMSMSX101 dot amr dot corp dot intel dot com> <51C0F06E dot 3080507 at redhat dot com> <BF230D13CA30DD48930C31D4099330003A439A73 at FMSMSX101 dot amr dot corp dot intel dot com> <51C341D2 dot 8020707 at redhat dot com> <BF230D13CA30DD48930C31D4099330003A43A479 at FMSMSX101 dot amr dot corp dot intel dot com> <51C5CEC9 dot 8050309 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