RE: [PATCH] _Cilk_for for C and C++

["Iyer, Balaji V" <balaji dot v dot iyer at intel dot com>]

Hi Jason,
	I am attaching a fixed patch. I have resolved all the issues you have mentioned below and I have added answers to your questions below. I have not regenerated the C patch since nothing has changed on it.

Here are the ChangeLog entries:
gcc/cp/ChangeLog.
2013-11-25  Balaji V. Iyer  <balaji.v.iyer@intel.com>

        * cp-cilkplus.c: Added cgraph.h, gimple.h and gimplify.h.
        (callable): New function.
        (calc_count_up_count_down): Likewise.
        (compute_loop_var_cp_iter_hdl): Likewise.
        (cp_create_cilk_for_body): Likewise.
        (create_cilk_for_nested_fn): Likewise.
        (gimplify_cilk_for_1): Likewise.
        (cp_extract_cilk_for_fields): Likewise.
        (cp_gimplify_cilk_for): Likewise.
        * cp-gimplify.c (genericize_cilk_for_stmt): Likewise.
        (cp_genericize_r): Added a check for CILK_FOR_STMT.
        * cp-objcp-common.h (LANG_HOOKS_CILKPLUS_GIMPLIFY_CILK_FOR): New
        #define.
        * cp-tree.h (begin_cilk_for_stmt): New prototype.
        (finish_cilk_for_stmt): Likewise.
        (finish_cilk_for_init_stmt): Likewise.
        (cp_gimplify_cilk_for): Likewise.
        * parser.c (cp_parser_cilk_grainsize): New function and prototype.
        (cp_parser_init_declarator): Added a new parameter to hold the
        initial value.
        (cp_parser_statement): Added RID_CILK_FOR case.
        (cp_parser_iteration_statement): Likewise.
        (cp_parser_jump_statement): Added IN_CILK_FOR_STMT case (twice).
        (cp_parser_pragma): Added PRAGMA_CILK_GRAINSIZE case.
        (cp_parser_cilk_for_init_statement): New function.
        (cp_parser_cilk_for): Renamed a parameter and added support for
        parsing _Cilk_for loops that are part of Cilk keywords.
        * parser.h (IN_CILK_FOR_STMT): New #define.
        * pt.c (tsubst_expr): Added CILK_FOR_STMT case.
        * semantics.c (begin_for_scope): Added "_Cilk_for statement" in the
        header comment.
        (finish_for_expr): Added support for CILK_FOR_STMT to use this
        function.
        (finish_cilk_for_cond): Added support for processing templates.
        (begin_cilk_for_stmt): New function.
        (finish_cilk_for_init_stmt): Likewise.
        (finish_clk_for_stmt): Likewise.

gcc/testsuite/ChangeLog.
2013-11-15  Balaji V. Iyer  <balaji.v.iyer@intel.com>

        * g++.dg/cilk-plus/CK/cilk-for-start-at-5.cc: New test.
        * g++.dg/cilk-plus/CK/cilk-for-tplt.cc: Likewise.
        * g++.dg/cilk-plus/CK/cilk-for.cc: Likewise.
        * g++.dg/cilk-plus/CK/cilk_for_cont_inside_for.cc: Likewise.
        * g++.dg/cilk-plus/CK/cilk_for_cont_with_for.cc: Likewise.
        * g++.dg/cilk-plus/CK/cilk_for_cont_with_if.cc: Likewise.
        * g++.dg/cilk-plus/CK/cilk_for_cont_with_while.cc: Likewise.
        * g++.dg/cilk-plus/CK/cilk_for_genricize_test.cc: Likewise.
        * g++.dg/cilk-plus/CK/cilk_for_grainsize.cc: Likewise.
        * g++.dg/cilk-plus/CK/cilk_for_p_errors.cc: Likewise.
        * g++.dg/cilk-plus/CK/cilk_for_t_errors.cc: Likewise.
        * g++.dg/cilk-plus/CK/explicit_ctor.cc: Likewise.
        * g++.dg/cilk-plus/CK/label_test.cc: Likewise.
        * g++.dg/cilk-plus/CK/no-opp-overload-error.cc: Likewise.
        * g++.dg/cilk-plus/CK/plus-equal-one.cc: Likewise.
        * g++.dg/cilk-plus/CK/plus-equal-test.cc: Likewise.
        * g++.dg/cilk-plus/CK/stl_iter.cc: Likewise.
        * g++.dg/cilk-plus/CK/stl_test.cc: Likewise.
        * g++.dg/cilk-plus/cilk-plus.exp: Added support to call _Cilk_for
        testcodes.

Thanks,

Balaji V. Iyer.

> -----Original Message-----
> From: Jason Merrill [mailto:jason@redhat.com]
> Sent: Friday, November 22, 2013 11:46 AM
> To: Iyer, Balaji V; Aldy Hernandez
> Cc: gcc-patches@gcc.gnu.org; Jeff Law; rth@redhat.com
> Subject: Re: [PATCH] _Cilk_for for C and C++
> 
> 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.
> 

Fixed. I used LOOKUP_NORMAL

> > +  tree exp = build_new_op (EXPR_LOCATION (op1), code, flags, op0, op1,
> > +			   NULL_TREE, NULL, 0);
> 
> Use tf_none instead of 0.
> 

Fixed.

> > +  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?
> 

Yes, that was a mistake on my side. The exp cannot be error_mark_code at this point  I am sorry. It is removed.

> > +/* 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?
> 

var2 is the copy of the induction variable in _Cilk_for but its context is the cilk_for nested function.

> > +      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?
> 

This part is fixed as I mentioned above.

> > +  /* 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.
> 

You are correct, I don't need to do them. I just need to emit a FOR_STMT with the body inside it and then when I do a cp_genericize, it will automatically resolve it. I fixed it accordingly.

> > +     b. C++ doesn't provide a c_finish_loop function like C does.  */
> 
> Why is that important?
> 

Please see my note above. By the way, I hope you didn't read my above comment as knocking the C++ implementation. I just gave a detailed explanation as to why I did the loop-creation by hand. I have removed that now since it is no longer applicable

> >    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.
> 

Yep. Removed.

> Jason

Attachment: diff.txt
Description: diff.txt