Hello Joseph,
Thanks for reviewing my patch. Please see my responses below:
>-----Original Message-----
>From: Joseph Myers [mailto:jos...@codesourcery.com]
>Sent: Wednesday, September 05, 2012 8:07 PM
>To: Iyer, Balaji V
>Cc: gcc-patches@gcc.gnu.org; Aldy Hernandez (al...@redhat.com); Jeff Law;
>r...@redhat.com
>Subject: Re: [PATCH] Merging Cilk Plus into Trunk (Patch 1 of approximately 22)
>
>On Wed, 5 Sep 2012, Iyer, Balaji V wrote:
>
>> Attached, please find the 1st of ~22 patches that implements Cilk
>> Plus. This patch will implement Elemental Functions into the C compiler.
>> Please check it in to the trunk if it looks OK.
>>
>> Below, I will give you a small example about what elemental function
>> is and how it can be useful. Details about elemental function can be
>> found in the following link
>> (http://software.intel.com/en-us/articles/elemental-functions-writing-
>> data-parallel-code-in-cc-using-intel-cilk-plus)
>
>That page says "To continue reading the article, click on the link below."
>but I don't see such a link below.
Sorry about that. We were recently updating the website and I think it may have
gotten messed up during then. I have contacted the appropriate people to fix
it. I will let you know as soon as the link is fixed.
>
>> * c-cpp-elem-function.c (create_processor_attribute): Likewise.
>
>I don't see a ChangeLog entry for the addition of this file at all. When a
>new file
>is added, "New file." is enough entry; you don't describe particular things
>within
>the file.
Ok, I was mistaken there. I thought we had to add a changelog entry for every
function and not every file. I will fix it in the updated patch I send soon.
>
>This file includes tm.h and tm_p.h. Inclusion of these headers from front-end
>code is deprecated. If they are really needed, please put comments on the
>includes about exactly what target macros are being used in this front-end
>code.
>Similarly, use of hard-reg-set.h in front-end code is doubtful. Generally,
>please
>check all #includes in all new source files and make sure that each include is
>actually needed because some functionality from the relevant header is used in
>the source file; do not just copy the headers included by some existing source
>file.
OK, I will look into this. I don't believe I am using tm_p.h or tm.h. I just
put them in just in case.
>
>create_processor_attribute contains hardcoded references to x86-specific
>functionality. This is not OK; all such target dependencies need to be kept
>within
>the back ends, and handled from the rest of the compiler via target hooks (in
>most cases, new target dependencies must use target hooks not target macros).
The only thing I am doing in that function is to add appropriate attribute. In
elemental function, there is a processor clause that will allow users to set
the type of processor they want the function compiled for. All I am doing is to
map that information to the appropriate "arch" attribute. I didn't think it had
any back end pecularity.
>
>Please make sure every new function has a comment explicitly describing the
>semantics of every parameter and the return value as well as anything else the
>function does.
I was trying to follow suit with other functions nearby. Most function had 1 or
2 line header comments that gives a quick description about the function.
>
>Where there are alternative versions of functions/macros with/without explicit
>locations, please use the forms with explicit locations (e.g.
>build2_loc instead of build2), and try to link the locations to particular
>source
>code tokens and pass those locations down explicitly to each function as
>needed.
I have tried to preserve the location wherever appropriate. In many cases I
used UNKNOWN_LOCATION or omitted the location information because the code is
internally generated which does not have a line number.
>
>There may be more issues; I'll await a revised patch before doing further
>review.
I will send another one ASAP.
>
>--
>Joseph S. Myers
>jos...@codesourcery.com
Yours Sincerely,
Balaji V. Iyer.
