> -----Original Message----- > From: Richard Henderson [mailto:r...@redhat.com] > Sent: Wednesday, May 22, 2013 1:34 PM > To: Iyer, Balaji V > Cc: 'Joseph S. Myers'; 'Aldy Hernandez'; 'gcc-patches' > Subject: Re: [PING]RE: [patch] cilkplus: Array notation for C patch > > On 2013-05-22 08:18, Iyer, Balaji V wrote: > > 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. > > Really, that's surprising: > > /* For use from the C common bits. */ > tree > build_modify_expr (location_t /*location*/, > tree lhs, tree /*lhs_origtype*/, > enum tree_code modifycode, > location_t /*rhs_location*/, tree rhs, > tree /*rhs_origtype*/) { > return cp_build_modify_expr (lhs, modifycode, rhs, tf_warning_or_error); } > > I can definitely see how function overloading could potentially get in the > way.
There are couple other ones such as build_x_unary_op, build_x_conditional_expr which does not have such a mapping that I know of. > > Although by delaying expansion of ARRAY_NOTATION_REF, given that the > TREE_TYPE of the ARRAY_NOTATION_REF is not some complex artificial slice > type but the element type of the array, I wonder how much of the overloaded > function selection would just happen automatically? > > Which reminds me: What is ARRAY_NOTATION_TYPE for? Isn't it always the > same as the TREE_TYPE of the ARRAY_NOTATION_REF? Yes, they are both the same. A while back, I found a couple corner cases where the TREE_TYPE of the array notations inside __sec_reduce functions that was getting changed. This is a storage location that will be untouched so that I can get the original type. > > > 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. > > Well, append_to_statement_list etc was certainly not needed. Mirroring > exactly > how you'd write it in C, with a comma operator also works. That's a > COMPOUND_EXPR, fyi. > > There are continue/break label arguments to c_finish_loop, but they may be > (and often are) null. They will only be non-null if the relevant statements > actually exist inside the loop. C.f. c_finish_bc_stmt and c_break_label. > > You're right about it not saving *that* much code, but the point is more > about if > we make improvements to normal C loop representation -- such as to support > #pragma simd -- then we want to be able to take advantage of that here. OK. I see your point. I will look into changing them. > > > 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? > > I'm leaning to honoring the advice you were given last year and accepting the > patch more or less in its current form. Thank you! > > > r~
