Hello Richard,
Thank you for reviewing my code. Please see my responses below.
Thanks,
Balaji V. Iyer.
> -----Original Message-----
> From: Richard Henderson [mailto:r...@redhat.com]
> Sent: Tuesday, May 21, 2013 10:57 PM
> To: Iyer, Balaji V
> Cc: 'Joseph S. Myers'; 'Aldy Hernandez'; 'gcc-patches'
> Subject: Re: [PING]RE: [patch] cilkplus: Array notation for C patch
>
> Let me start by saying that I think the patch is generally ok, especially
> considering the advice that's already been given. That said...
>
> > +++ b/gcc/c/c-array-notation.c
> > @@ -0,0 +1,3121 @@
>
> So, like, are we going to need to replicate 3000 lines to add array notation
> to
> c++ too? How much of this are we going to be able to share?
The overall function names are same, but the components inside it function
differs greatly from C and C++. For example, in C++ I can't use
build_modify_expr, but build_x_modify_expr. Also, I need to handle overloaded
function types, and that requires further checking. Similarly, the trees are
different from C and C++, and so I have to do more checks and handle them
differently in C++. In my mind, this seem to be the most straight-forward and
legible way to do it.
>
> While I don't think we should make the array notation global, we might be able
> to share the code via c-family/. The Ideal way I think would be via
> c-gimplify, in
> which we don't expand the array notation until later. But even if delaying
> the
> expansion causes other problems that I havn't thought about, just placing the
> common expansion logic in the shared directory would be better.
>
> > + for (ii = 0; ii < rank ; ii++)
> > + {
> > + /* This will create the if statement label. */
> > + if_stmt_label[ii] = build_decl (location, LABEL_DECL, NULL_TREE,
> > + void_type_node);
> > + DECL_CONTEXT (if_stmt_label[ii]) = current_function_decl;
> > + DECL_ARTIFICIAL (if_stmt_label[ii]) = 0;
> > + DECL_IGNORED_P (if_stmt_label[ii]) = 1;
> > +
> > + /* This label statement will point to the loop body. */
> > + body_label[ii] = build_decl (location, LABEL_DECL, NULL_TREE,
> > + void_type_node);
> > + DECL_CONTEXT (body_label[ii]) = current_function_decl;
> > + DECL_ARTIFICIAL (body_label[ii]) = 0;
> > + DECL_IGNORED_P (body_label[ii]) = 1;
> > + body_label_expr[ii] = build1 (LABEL_EXPR, void_type_node,
> > + body_label[ii])
> > ;
> > +
> > + /* This will create the exit label, i.e. where the while loop will
> > branch
> > + out of. */
> > + exit_label[ii] = build_decl (location, LABEL_DECL, NULL_TREE,
> > + void_type_node);
> > + DECL_CONTEXT (exit_label[ii]) = current_function_decl;
> > + DECL_ARTIFICIAL (exit_label[ii]) = 0;
> > + DECL_IGNORED_P (exit_label[ii]) = 1;
> > + exit_label_expr[ii] = build1 (LABEL_EXPR, void_type_node,
> > exit_label[ii]);
> > + }
>
> Is there any particular reason why you're open-coding the loop expansion
> instead of using existing infrastructure like c_finish_loop?
Before I started my implementation, I did briefly consider using c_finish_loop,
but adding multiple expressions got a bit complicated. For example, if I have 2
increment/condition expressions (my initial implementation had it but later on
I eliminated that need), I had to create a statement list and then add them
using append_to_statement_list() and then pass that into c_finish_loop. Also,
if I am not mistaken, I still had to create some of the individual labels when
using c_finish_loop(). Overall, I didn't find much code-size decrease, and I
found this a bit more straight forward for me. I apologize if I am mistaken.
>
> Yes, the specific example of c_finish_loop goes against the c++ sharing, but
> it's
> fairly easy to add new interfaces to c-common.h that are implemented in the
> two front ends.
For C++, I looked into creating FOR_STMT instead. I found this slightly more
convenient than c_finish_loop, but decided to stick with this approach for
convenience, and I didn't find much code-size decrease.
>
Are these changes a blocking issue for acceptance into trunk? Or, would it be
ok to accept it as-is and then I make these changes at a later date?
>
>
> r~
