llvm dev - Oct 2012 - [LLVMdev] [cfe-dev] OpenMP support in CLANG: A proposal

On Sat, Oct 13, 2012 at 2:33 AM, Mahesha HS <mahesha.llvm at gmail.com>
wrote:
> On Sat, Oct 13, 2012 at 1:31 PM, Tobias Grosser <tobias at
grosser.es> wrote:
>> On 10/13/2012 04:38 AM, Mahesha HS wrote:
>>>
>>> On Sat, Oct 13, 2012 at 5:14 AM, Eli Friedman <eli.friedman at
gmail.com>
>>> wrote:
>>>>
>>>> On Wed, Oct 10, 2012 at 6:15 AM, Mahesha HS <mahesha.llvm at
gmail.com>
>>>> wrote:
>>>>>
>>>>> Hi Eli and Others
>>>>>
>>>>> In response to your feedback, I have taken care of all your
review
>>>>> comments
>>>>> - I removed clangOMP.a
>>>>> and moved the implementation of "class
OmpPragmaHandler" to
>>>>> "clangLex.a".
>>>>>
>>>>> The attached zipped file - namely -
'OpenMP_support_in_Clang.tar.gz'
>>>>> contains all the implemented
>>>>> "patches" along with *newly* added source files.
>>>>>
>>>>> Another attached text file - namely -
>>>>> 'OpenMP_support_in_Clang_Read_Me.txt'
>>>>> briefly describes the
>>>>> implementation.
>>>>>
>>>>> You can also refer to the initial proposal document -
namely -
>>>>> 'OpenMP_Support_in_Clang.pdf'.
>>>>>
>>>>> Looking forward for your review comments.
>>>>
>>>>
>>>> Can you please split this patch somehow?  This is way too big
to
>>>> easily review.  I'd like to see separate patches for the
command-line
>>>> option, the pragma handler itself, parsing each pragma, each
new AST
>>>> node, and semantic analysis for each kind of pragma, with
testcases
>>>> for every patch.  And the more you can split them, the better
to get
>>>> through reviews faster.
>>>>
>>>> Please include new files in patches using "svn add";
don't send new
>>>> files separately.
>>>>
>>>> Please look over patches before submitting to make sure you
don't
>>>> include unnecessary changes to whitespace etc.
>>>
>>>
>>> Thanks for the reply. Sure, I will again send updated patches which
>>> take care of all the above comments.This is the first time, I am
>>> sending the patch to CLANG/LLVM community in particular, and to any
>>> open source community, in general. Also, I was in a hurry to send
the
>>> patch based on your first reply. Will be sending the updated
patches
>>> soon.
>>
>>
>> Hey Mahesh,
>>
>> thanks for working on this. As a small hint from my side, I have the
feeling
>> there is currently no need to submit the entire OpenMP patch for
review. You
>> could e.g. start with the patch that adds the (command
>> line option, but just ignores it. As a next step, you could submit a
patch
>> that makes  >>include "omp.h" << work (without
yet supporting
>> any definitions, ...)
>>
>> I think at the beginning it is helpful to start with smaller patches
and to
>> get used to the review policies. This will prepare you for the
>> larger discussions with Elli and Co. :-)
>
> Hi Tobias,
>
> Incremental reviews (and check-ins) make sense actually.  I think, it
> helps both me and Eli (and others) in several ways.
>
> @ Eli,
>
> Is it fine with you too?
Yes, please do.

-Eli

Hi Eli,

Attached zipped file, named, "fopenmp_option_support.tar.gz" contains
the first patch, along with relevant *test case*. This patch is to
support the option "-fopenmp" option in Clang.

Following files are changed in this patch. Please start going through
this patch, and let me know comments. Meanwhile, I will prepare next
patch.

========================================#. clang/include/clang/Driver/Options.td
#. clang/include/clang/Basic/LangOptions.def
#. clang/include/clang/Basic/DiagnosticLexKinds.td
 #. clang/lib/Driver/Tools.cpp
 #. clang/lib/Frontend/CompilerInvocation.cpp
========================================
--
mahesha



On Sat, Oct 13, 2012 at 3:39 PM, Eli Friedman <eli.friedman at gmail.com>
wrote:
> On Sat, Oct 13, 2012 at 2:33 AM, Mahesha HS <mahesha.llvm at
gmail.com> wrote:
>> On Sat, Oct 13, 2012 at 1:31 PM, Tobias Grosser <tobias at
grosser.es> wrote:
>>> On 10/13/2012 04:38 AM, Mahesha HS wrote:
>>>>
>>>> On Sat, Oct 13, 2012 at 5:14 AM, Eli Friedman <eli.friedman
at gmail.com>
>>>> wrote:
>>>>>
>>>>> On Wed, Oct 10, 2012 at 6:15 AM, Mahesha HS
<mahesha.llvm at gmail.com>
>>>>> wrote:
>>>>>>
>>>>>> Hi Eli and Others
>>>>>>
>>>>>> In response to your feedback, I have taken care of all
your review
>>>>>> comments
>>>>>> - I removed clangOMP.a
>>>>>> and moved the implementation of "class
OmpPragmaHandler" to
>>>>>> "clangLex.a".
>>>>>>
>>>>>> The attached zipped file - namely -
'OpenMP_support_in_Clang.tar.gz'
>>>>>> contains all the implemented
>>>>>> "patches" along with *newly* added source
files.
>>>>>>
>>>>>> Another attached text file - namely -
>>>>>> 'OpenMP_support_in_Clang_Read_Me.txt'
>>>>>> briefly describes the
>>>>>> implementation.
>>>>>>
>>>>>> You can also refer to the initial proposal document -
namely -
>>>>>> 'OpenMP_Support_in_Clang.pdf'.
>>>>>>
>>>>>> Looking forward for your review comments.
>>>>>
>>>>>
>>>>> Can you please split this patch somehow?  This is way too
big to
>>>>> easily review.  I'd like to see separate patches for
the command-line
>>>>> option, the pragma handler itself, parsing each pragma,
each new AST
>>>>> node, and semantic analysis for each kind of pragma, with
testcases
>>>>> for every patch.  And the more you can split them, the
better to get
>>>>> through reviews faster.
>>>>>
>>>>> Please include new files in patches using "svn
add"; don't send new
>>>>> files separately.
>>>>>
>>>>> Please look over patches before submitting to make sure you
don't
>>>>> include unnecessary changes to whitespace etc.
>>>>
>>>>
>>>> Thanks for the reply. Sure, I will again send updated patches
which
>>>> take care of all the above comments.This is the first time, I
am
>>>> sending the patch to CLANG/LLVM community in particular, and to
any
>>>> open source community, in general. Also, I was in a hurry to
send the
>>>> patch based on your first reply. Will be sending the updated
patches
>>>> soon.
>>>
>>>
>>> Hey Mahesh,
>>>
>>> thanks for working on this. As a small hint from my side, I have
the feeling
>>> there is currently no need to submit the entire OpenMP patch for
review. You
>>> could e.g. start with the patch that adds the (command
>>> line option, but just ignores it. As a next step, you could submit
a patch
>>> that makes  >>include "omp.h" << work
(without yet supporting
>>> any definitions, ...)
>>>
>>> I think at the beginning it is helpful to start with smaller
patches and to
>>> get used to the review policies. This will prepare you for the
>>> larger discussions with Elli and Co. :-)
>>
>> Hi Tobias,
>>
>> Incremental reviews (and check-ins) make sense actually.  I think, it
>> helps both me and Eli (and others) in several ways.
>>
>> @ Eli,
>>
>> Is it fine with you too?
>
> Yes, please do.
>
> -Eli


-- 
mahesha
-------------- next part --------------
A non-text attachment was scrubbed...
Name: fopenmp_option_support.tar.gz
Type: application/x-gzip
Size: 1550 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20121013/9cbf59f1/attachment.bin>

Hi Eli,

Attached is the next patch in the line. This patch
(class_pragma_omp_handler_support.patch) contains the implementation
of the class "class PragmaOmpHandler".  I also attached the test case
(openmp_syntax_test.c).  This test case is actually to test the
syntactically legal simple OpenMP constructs. However, we can *really*
test it only after submitting the next two patches - one related to
PragmaOmpHandler object construction and the another related to OpenMP
parsing.

Please, let me know, if you need to know any additional information.

If you think that some other mechanism is required to speed-up the
review process, I will really welcome it.


--
mahesha


On Sat, Oct 13, 2012 at 9:36 PM, Mahesha HS <mahesha.llvm at gmail.com>
wrote:
> Hi Eli,
>
> Attached zipped file, named, "fopenmp_option_support.tar.gz"
contains
> the first patch, along with relevant *test case*. This patch is to
> support the option "-fopenmp" option in Clang.
>
> Following files are changed in this patch. Please start going through
> this patch, and let me know comments. Meanwhile, I will prepare next
> patch.
>
> ========================================> #.
clang/include/clang/Driver/Options.td
> #. clang/include/clang/Basic/LangOptions.def
> #. clang/include/clang/Basic/DiagnosticLexKinds.td
>  #. clang/lib/Driver/Tools.cpp
>  #. clang/lib/Frontend/CompilerInvocation.cpp
> ========================================>
> --
> mahesha
>
>
>
> On Sat, Oct 13, 2012 at 3:39 PM, Eli Friedman <eli.friedman at
gmail.com> wrote:
>> On Sat, Oct 13, 2012 at 2:33 AM, Mahesha HS <mahesha.llvm at
gmail.com> wrote:
>>> On Sat, Oct 13, 2012 at 1:31 PM, Tobias Grosser <tobias at
grosser.es> wrote:
>>>> On 10/13/2012 04:38 AM, Mahesha HS wrote:
>>>>>
>>>>> On Sat, Oct 13, 2012 at 5:14 AM, Eli Friedman
<eli.friedman at gmail.com>
>>>>> wrote:
>>>>>>
>>>>>> On Wed, Oct 10, 2012 at 6:15 AM, Mahesha HS
<mahesha.llvm at gmail.com>
>>>>>> wrote:
>>>>>>>
>>>>>>> Hi Eli and Others
>>>>>>>
>>>>>>> In response to your feedback, I have taken care of
all your review
>>>>>>> comments
>>>>>>> - I removed clangOMP.a
>>>>>>> and moved the implementation of "class
OmpPragmaHandler" to
>>>>>>> "clangLex.a".
>>>>>>>
>>>>>>> The attached zipped file - namely -
'OpenMP_support_in_Clang.tar.gz'
>>>>>>> contains all the implemented
>>>>>>> "patches" along with *newly* added source
files.
>>>>>>>
>>>>>>> Another attached text file - namely -
>>>>>>> 'OpenMP_support_in_Clang_Read_Me.txt'
>>>>>>> briefly describes the
>>>>>>> implementation.
>>>>>>>
>>>>>>> You can also refer to the initial proposal document
- namely -
>>>>>>> 'OpenMP_Support_in_Clang.pdf'.
>>>>>>>
>>>>>>> Looking forward for your review comments.
>>>>>>
>>>>>>
>>>>>> Can you please split this patch somehow?  This is way
too big to
>>>>>> easily review.  I'd like to see separate patches
for the command-line
>>>>>> option, the pragma handler itself, parsing each pragma,
each new AST
>>>>>> node, and semantic analysis for each kind of pragma,
with testcases
>>>>>> for every patch.  And the more you can split them, the
better to get
>>>>>> through reviews faster.
>>>>>>
>>>>>> Please include new files in patches using "svn
add"; don't send new
>>>>>> files separately.
>>>>>>
>>>>>> Please look over patches before submitting to make sure
you don't
>>>>>> include unnecessary changes to whitespace etc.
>>>>>
>>>>>
>>>>> Thanks for the reply. Sure, I will again send updated
patches which
>>>>> take care of all the above comments.This is the first time,
I am
>>>>> sending the patch to CLANG/LLVM community in particular,
and to any
>>>>> open source community, in general. Also, I was in a hurry
to send the
>>>>> patch based on your first reply. Will be sending the
updated patches
>>>>> soon.
>>>>
>>>>
>>>> Hey Mahesh,
>>>>
>>>> thanks for working on this. As a small hint from my side, I
have the feeling
>>>> there is currently no need to submit the entire OpenMP patch
for review. You
>>>> could e.g. start with the patch that adds the (command
>>>> line option, but just ignores it. As a next step, you could
submit a patch
>>>> that makes  >>include "omp.h" << work
(without yet supporting
>>>> any definitions, ...)
>>>>
>>>> I think at the beginning it is helpful to start with smaller
patches and to
>>>> get used to the review policies. This will prepare you for the
>>>> larger discussions with Elli and Co. :-)
>>>
>>> Hi Tobias,
>>>
>>> Incremental reviews (and check-ins) make sense actually.  I think,
it
>>> helps both me and Eli (and others) in several ways.
>>>
>>> @ Eli,
>>>
>>> Is it fine with you too?
>>
>> Yes, please do.
>>
>> -Eli
>
>
>
> --
> mahesha


-- 
mahesha
-------------- next part --------------
A non-text attachment was scrubbed...
Name: class_pragma_omp_handler_support.patch
Type: application/octet-stream
Size: 44887 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20121016/08269b3b/attachment.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: openmp_syntax_test.c
Type: text/x-csrc
Size: 14145 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20121016/08269b3b/attachment.c>

On Sat, Oct 13, 2012 at 9:06 AM, Mahesha HS <mahesha.llvm at gmail.com>
wrote:
> Hi Eli,
>
> Attached zipped file, named, "fopenmp_option_support.tar.gz"
contains
> the first patch, along with relevant *test case*. This patch is to
> support the option "-fopenmp" option in Clang.
>
> Following files are changed in this patch. Please start going through
> this patch, and let me know comments. Meanwhile, I will prepare next
> patch.
In the future, please include tests inside the .patch file (you can
use svn add to tell svn to include it in diffs).

// clang should *not* emit the below warning:
// warning: argument unused during compilation: '-fopenmp'
//
// RUN: %clang -c %s 2> %t.log
//
// RUN: grep "warning: argument unused during compilation:
'-fopenmp'"
%t.log | count 0

This test is way too specific to be useful.

A test that checks the appropriate option is passed to clang -cc1
would be appropriate.

+def omp_pragma_ignored : Warning<
+  "omp pragma ignored; did you forget to add '-fopenmp'
flag?">;

This warning appears to be unused.  (Please try to add warnings in the
same patch they are used.)

+LANGOPT(OpenMpOn, 1, 0, "Enables OpenMP support.")

Please just name this OpenMP; the "On" doesn't add anything.

+  // Support for OpenMP: Implementation of 'fopenmp' option passing
+  // mechanism.
+  if (Opts.OpenMpOn)
+    Res.push_back("-fopenmp");

This is overly verbose; a very short comment like "OpenMP support", or
even no comment at all, would be appropriate.

Please send future patches to cfe-commits; these patches aren't of
general interest to all of cfe-dev and llvmdev.

-Eli