This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH] _Cilk_for for C and C++
- From: Jason Merrill <jason at redhat dot com>
- To: "Iyer, Balaji V" <balaji dot v dot iyer at intel dot com>, Aldy Hernandez <aldyh at redhat dot com>
- Cc: "gcc-patches at gcc dot gnu dot org" <gcc-patches at gcc dot gnu dot org>, Jeff Law <law at redhat dot com>, "rth at redhat dot com" <rth at redhat dot com>
- Date: Fri, 22 Nov 2013 11:45:54 -0500
- Subject: Re: [PATCH] _Cilk_for for C and C++
- Authentication-results: sourceware.org; auth=none
- References: <BF230D13CA30DD48930C31D4099330003A49F0C1 at FMSMSX101 dot amr dot corp dot intel dot com> <52869727 dot 4060307 at redhat dot com> <BF230D13CA30DD48930C31D4099330003A4A8DA6 at FMSMSX101 dot amr dot corp dot intel dot com>
On 11/18/2013 04:50 PM, Iyer, Balaji V wrote:
+ int flags = LOOKUP_PROTECT | LOOKUP_ONLYCONVERTING;
Why not LOOKUP_NORMAL? LOOKUP_ONLYCONVERTING isn't relevant in this context.
+ tree exp = build_new_op (EXPR_LOCATION (op1), code, flags, op0, op1,
+ NULL_TREE, NULL, 0);
Use tf_none instead of 0.
+ if (exp == error_mark_node)
+ exp = build_x_modify_expr (EXPR_LOCATION (op1), op0, code, op1, tf_none);
+ if (exp && exp != error_mark_node)
+ return exp;
This doesn't make sense to me. build_x_modify_expr takes codes like
PLUS_EXPR and then does an assignment afterward; we don't want to
quietly do += just because there's some error with the evaluation of the
+ operation. What is this code trying to do?
+/* Handler for iterator to compute the loop variable. ADD_OP indicates
+ whether we need a '+' or '-' operation. LOW indicates the starting point
+ and LOOP_VAR is the induction variable. Returns an expression (or a
+ STATEMENT_LIST of expressions). If it is unable to find the appropriate
+ iteration, then it returns an error mark node and its parent will set
+ the loop as invalid. */
This doesn't explain what VAR2 is. And it seems like you're also using
LOW as the increment?
+ tree new_stmt = build_x_modify_expr (loc, new_var, INIT_EXPR,
+ build_zero_cst (TREE_TYPE (new_var)),
+ tf_warning_or_error);
+ if (new_stmt == error_mark_node)
+ return error_mark_node;
+ append_to_statement_list (new_stmt, &exp);
+ new_stmt = build_x_modify_expr (loc, new_var, NOP_EXPR, low,
+ tf_warning_or_error);
Why assign 0 if you're going to immediately assign low afterwards?
+ /* We have to manually create this loop for two reasons:
+ a. We need to have access to continue and start label since we need
+ to resolve continue and breaks by hand.
Why do you need to resolve them by hand? It looks like break isn't even
allowed.
+ b. C++ doesn't provide a c_finish_loop function like C does. */
Why is that important?
sk_for, /* The scope of the variable declared in a
for-init-statement. */
+ sk_cilk_for, /* The scope of the variable declared in _Cilk_for init
+ statement. */
How is this different from a normal for-init-statement? Nothing seems
to use it.
Jason