OpenMP support in CLANG: A proposal

classic Classic list List threaded Threaded
38 messages Options
12
Reply | Threaded
Open this post in threaded view
|

OpenMP support in CLANG: A proposal

Mahesha HS
Hello All,

We would like to make a proposal to support OpenMP in CLANG. The goal of this effort is to provide support for syntax

analysis (parsing), semantic analysis and AST implementation for OpenMP constructs in CLANG.

 

We would like to defer the design for *lowering* based on the outcome of the discussion happening in the LLVM dev list.

Our design is aimed at implementing the necessary support in CLANG irrespective of how it is finally *lowered*.

 

Please find the details of the proposal and the current status in the document attached.

 

Looking forward for your constructive feedback.



--
mahesha


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev

OpenMP_Support_in_Clang.pdf (264K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: OpenMP support in CLANG: A proposal

Eli Friedman
On Tue, Oct 9, 2012 at 4:37 AM, Mahesha HS <[hidden email]> wrote:

> Hello All,
>
> We would like to make a proposal to support OpenMP in CLANG. The goal of
> this effort is to provide support for syntax
>
> analysis (parsing), semantic analysis and AST implementation for OpenMP
> constructs in CLANG.
>
>
>
> We would like to defer the design for *lowering* based on the outcome of the
> discussion happening in the LLVM dev list.
>
> Our design is aimed at implementing the necessary support in CLANG
> irrespective of how it is finally *lowered*.
>
>
>
> Please find the details of the proposal and the current status in the
> document attached.

Please don't add a separate clangOMP.a; you're implementing new
parsing and semantic analysis, but it isn't conceptually separate from
the existing parsing/semantic analysis.

I'm not entirely sure what sort of feedback you're expecting; ignoring
the lowering, the part of implementing OpenMP that's likely to attract
discussion is the AST representation, and you haven't described that
in any detail.

If you have patches that implement useful functionality; please submit
sooner rather than later.  Doing a bunch of work in a private branch
will mean more work for you in the long run because you won't get any
feedback.

-Eli\
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: OpenMP support in CLANG: A proposal

Mahesha HS


On Wed, Oct 10, 2012 at 5:40 AM, Eli Friedman <[hidden email]> wrote:
On Tue, Oct 9, 2012 at 4:37 AM, Mahesha HS <[hidden email]> wrote:
> Hello All,
>
> We would like to make a proposal to support OpenMP in CLANG. The goal of
> this effort is to provide support for syntax
>
> analysis (parsing), semantic analysis and AST implementation for OpenMP
> constructs in CLANG.
>
>
>
> We would like to defer the design for *lowering* based on the outcome of the
> discussion happening in the LLVM dev list.
>
> Our design is aimed at implementing the necessary support in CLANG
> irrespective of how it is finally *lowered*.
>
>
>
> Please find the details of the proposal and the current status in the
> document attached.

Please don't add a separate clangOMP.a; you're implementing new
parsing and semantic analysis, but it isn't conceptually separate from
the existing parsing/semantic analysis.

First, I would like to clarify that clangOMP.a implements only, *and only*
the "class OmpPragmaHandler" and nothing else. Parsing and Semantic
Analysis is as usual will be done by clangParse.a and clangSema.a components
respectively.

Second, the reason for adding a separate clangOMP.a. is as follows. I looked into the
Clang code base, where few *pragmas* already being supported. I noticed that
few pragmas like "\#pragma once" are implemented in "clangLex.a", and some other
pragmas like "\#pragma align" are implemented in "clangParse.a". Also, I thought,
OpenMP includes many pragma directives and clauses, so better to separate it out in
clangOMP.a.

However, your feedback is well considered. I will remove clangOMP.a and move the
implementation of "class OmpPragmaHandler" to "clangLex.a".
 

I'm not entirely sure what sort of feedback you're expecting; ignoring
the lowering, the part of implementing OpenMP that's likely to attract
discussion is the AST representation, and you haven't described that
in any detail.

As we mentioned in the previous mail, we are currently, deferred the design for
*lowering* for the time being based on the outcome of the discussion happening
in LLVM dev list on the topic of supporting OpenMP in LLVM.

However, AST representation and implementation is completed for all OpenMP directives
and clauses, except for "critical" directive statement.

In the document, that we attached in our previous mail, we gave an idea of how the AST looks
for an OpenMP directive statement as we did not want to bloat-up the proposal document
with too many details. However, we are ready to share code base, which will give more
information to provide comments.

In summary, we have completed all the required *basic* infrastructure design and implementation
to implement OpenMP in Clang. We would like to get feedback on it before proceeding for
further support.



If you have patches that implement useful functionality; please submit
sooner rather than later.  Doing a bunch of work in a private branch
will mean more work for you in the long run because you won't get any
feedback.

We have all the functionality implemented to provide the basic infrastructure along with the
implementation of AST classes to represent different OpenMP directives and clauses,
and the implementation of the "class OmpPrgamaHandler" as described in the doc.

We can share this code very soon, probably by tomorrow.

--
mahesha
 

-Eli\



--
mahesha


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: OpenMP support in CLANG: A proposal

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

--
mahesha






On Wed, Oct 10, 2012 at 10:30 AM, Mahesha HS <[hidden email]> wrote:


On Wed, Oct 10, 2012 at 5:40 AM, Eli Friedman <[hidden email]> wrote:
On Tue, Oct 9, 2012 at 4:37 AM, Mahesha HS <[hidden email]> wrote:
> Hello All,
>
> We would like to make a proposal to support OpenMP in CLANG. The goal of
> this effort is to provide support for syntax
>
> analysis (parsing), semantic analysis and AST implementation for OpenMP
> constructs in CLANG.
>
>
>
> We would like to defer the design for *lowering* based on the outcome of the
> discussion happening in the LLVM dev list.
>
> Our design is aimed at implementing the necessary support in CLANG
> irrespective of how it is finally *lowered*.
>
>
>
> Please find the details of the proposal and the current status in the
> document attached.

Please don't add a separate clangOMP.a; you're implementing new
parsing and semantic analysis, but it isn't conceptually separate from
the existing parsing/semantic analysis.

First, I would like to clarify that clangOMP.a implements only, *and only*
the "class OmpPragmaHandler" and nothing else. Parsing and Semantic
Analysis is as usual will be done by clangParse.a and clangSema.a components
respectively.

Second, the reason for adding a separate clangOMP.a. is as follows. I looked into the
Clang code base, where few *pragmas* already being supported. I noticed that
few pragmas like "\#pragma once" are implemented in "clangLex.a", and some other
pragmas like "\#pragma align" are implemented in "clangParse.a". Also, I thought,
OpenMP includes many pragma directives and clauses, so better to separate it out in
clangOMP.a.

However, your feedback is well considered. I will remove clangOMP.a and move the
implementation of "class OmpPragmaHandler" to "clangLex.a".
 

I'm not entirely sure what sort of feedback you're expecting; ignoring
the lowering, the part of implementing OpenMP that's likely to attract
discussion is the AST representation, and you haven't described that
in any detail.

As we mentioned in the previous mail, we are currently, deferred the design for
*lowering* for the time being based on the outcome of the discussion happening
in LLVM dev list on the topic of supporting OpenMP in LLVM.

However, AST representation and implementation is completed for all OpenMP directives
and clauses, except for "critical" directive statement.

In the document, that we attached in our previous mail, we gave an idea of how the AST looks
for an OpenMP directive statement as we did not want to bloat-up the proposal document
with too many details. However, we are ready to share code base, which will give more
information to provide comments.

In summary, we have completed all the required *basic* infrastructure design and implementation
to implement OpenMP in Clang. We would like to get feedback on it before proceeding for
further support.



If you have patches that implement useful functionality; please submit
sooner rather than later.  Doing a bunch of work in a private branch
will mean more work for you in the long run because you won't get any
feedback.

We have all the functionality implemented to provide the basic infrastructure along with the
implementation of AST classes to represent different OpenMP directives and clauses,
and the implementation of the "class OmpPrgamaHandler" as described in the doc.

We can share this code very soon, probably by tomorrow.

--
mahesha
 

-Eli\



--
mahesha




--
mahesha


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev

OpenMP_support_in_Clang_Read_Me.txt (11K) Download Attachment
OpenMP_support_in_Clang.tar.gz (46K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: OpenMP support in CLANG: A proposal

Eli Friedman
On Wed, Oct 10, 2012 at 6:15 AM, Mahesha HS <[hidden email]> 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.

-Eli
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: OpenMP support in CLANG: A proposal

Mahesha HS
On Sat, Oct 13, 2012 at 5:14 AM, Eli Friedman <[hidden email]> wrote:

> On Wed, Oct 10, 2012 at 6:15 AM, Mahesha HS <[hidden email]> 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.

-mahesha


>
> -Eli



--
mahesha
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [LLVMdev] OpenMP support in CLANG: A proposal

Tobias Grosser-2
On 10/13/2012 04:38 AM, Mahesha HS wrote:

> On Sat, Oct 13, 2012 at 5:14 AM, Eli Friedman <[hidden email]> wrote:
>> On Wed, Oct 10, 2012 at 6:15 AM, Mahesha HS <[hidden email]> 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. :-)

Tobi



_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [LLVMdev] OpenMP support in CLANG: A proposal

Mahesha HS
On Sat, Oct 13, 2012 at 1:31 PM, Tobias Grosser <[hidden email]> wrote:

> On 10/13/2012 04:38 AM, Mahesha HS wrote:
>>
>> On Sat, Oct 13, 2012 at 5:14 AM, Eli Friedman <[hidden email]>
>> wrote:
>>>
>>> On Wed, Oct 10, 2012 at 6:15 AM, Mahesha HS <[hidden email]>
>>> 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?

--
mahesha

>
> Tobi
>
>
>



--
mahesha
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [LLVMdev] OpenMP support in CLANG: A proposal

Eli Friedman
On Sat, Oct 13, 2012 at 2:33 AM, Mahesha HS <[hidden email]> wrote:

> On Sat, Oct 13, 2012 at 1:31 PM, Tobias Grosser <[hidden email]> wrote:
>> On 10/13/2012 04:38 AM, Mahesha HS wrote:
>>>
>>> On Sat, Oct 13, 2012 at 5:14 AM, Eli Friedman <[hidden email]>
>>> wrote:
>>>>
>>>> On Wed, Oct 10, 2012 at 6:15 AM, Mahesha HS <[hidden email]>
>>>> 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
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [LLVMdev] OpenMP support in CLANG: A proposal

Mahesha HS
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 <[hidden email]> wrote:

> On Sat, Oct 13, 2012 at 2:33 AM, Mahesha HS <[hidden email]> wrote:
>> On Sat, Oct 13, 2012 at 1:31 PM, Tobias Grosser <[hidden email]> wrote:
>>> On 10/13/2012 04:38 AM, Mahesha HS wrote:
>>>>
>>>> On Sat, Oct 13, 2012 at 5:14 AM, Eli Friedman <[hidden email]>
>>>> wrote:
>>>>>
>>>>> On Wed, Oct 10, 2012 at 6:15 AM, Mahesha HS <[hidden email]>
>>>>> 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

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev

fopenmp_option_support.tar.gz (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [LLVMdev] OpenMP support in CLANG: A proposal

Mahesha HS
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 <[hidden email]> 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 <[hidden email]> wrote:
>> On Sat, Oct 13, 2012 at 2:33 AM, Mahesha HS <[hidden email]> wrote:
>>> On Sat, Oct 13, 2012 at 1:31 PM, Tobias Grosser <[hidden email]> wrote:
>>>> On 10/13/2012 04:38 AM, Mahesha HS wrote:
>>>>>
>>>>> On Sat, Oct 13, 2012 at 5:14 AM, Eli Friedman <[hidden email]>
>>>>> wrote:
>>>>>>
>>>>>> On Wed, Oct 10, 2012 at 6:15 AM, Mahesha HS <[hidden email]>
>>>>>> 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

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev

class_pragma_omp_handler_support.patch (59K) Download Attachment
openmp_syntax_test.c (18K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [LLVMdev] OpenMP support in CLANG: A proposal

Eli Friedman
In reply to this post by Mahesha HS
On Sat, Oct 13, 2012 at 9:06 AM, Mahesha HS <[hidden email]> 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
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [LLVMdev] OpenMP support in CLANG: A proposal

Eli Friedman
In reply to this post by Mahesha HS
On Tue, Oct 16, 2012 at 4:11 AM, Mahesha HS <[hidden email]> wrote:

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

At this point, it's just a matter of finding time to review it.

+  // \Brief: Holds all the OpenMP clause name strings.
+  // \Brief: These strings are referenced while parsing OpenMP clauses.
+  // Note that we won't introduce new *tokens* for openMP clause names
+  // as these will get conflict with *identifier* token, and is very tricky
+  // to handle. Instead, we reference below strings to recognize the OpenMP
+  // clause names.
+  StringRef* Clause[END_CLAUSE];

This isn't correct documentation comment syntax; clang trunk has a
warning -Wdocumentation to catch this.

It's almost never appropriate to construct a StringRef*; a StringRef
is already essentially a pointer.

Is this array even necessary?  It's not clear what you're using it
for.  If you want to convert from the enum to a string, just implement
a method like "static StringRef StringForDefaultKind(ClauseKind
Kind);" or something like that.

+  // Note: These enum values *must* be in consistant in *size* and *order* with
+  // that of enum values defined in the AST node class "OmpClause".

If you want to share an enum, put it into a header in
include/clang/Basic/.  (A new header if no existing one is
appropriate.)  Having the same enum in multiple places is asking for
trouble.

This patch has a lot of copy-paste code; can you write a single
PragmaHandler subclass which is parameterized based on the pragma in
question?  (Or maybe a few, since it looks like some of them need
special handling which is not yet implemented?)

It looks like the only members of PragmaOmpHandler which actually need
to be in a header are PragmaOmpUnknownWarning, initialize method, and
the enums; please move PragmaOmpUnknownWarning and initialize onto the
Preprocessor class, the enums into a new header in
include/clang/Basic/, and everything else into the .cpp file, and
remove include/clang/Lex/PragmaOmpHandler.h.

-Eli
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [cfe-commits] [LLVMdev] OpenMP support in CLANG: A proposal

Mahesha HS
Dear All,

Based on the few code review comments that I received recently, I do
think that I should re-visit basic design that I had proposed
initially, and I want to make sure that the community is in agreement
with it, before I proceed further.

The very first design question to be discussed is about
*scanning/parsing* OpenMP names. As you know, there are so many OpenMP
specific names like parallel, for, private, shared, etc. And,
following are two possible choices to scan/parse them.

1.  Treat OpenMP names as *keywords*, introduce new lexical tokens for
each one them, make changes to Lexer/Parser to scan them as either
OpenMP keywords or as identifiers or as C/C++ keywords depending on
the context.

2.  Store all OpenMP names in a string table, and while parsing OpenMP
directive statements, perform string search and comparison in order to
parse them as either valid OpenMP names (upon successful string
search) or report syntax errors (upon unsuccessful string search).

[Any other possible choices?]

Both the above approaches have their pros and cons. First one looks
elegant at least theoretically, but requires changes to some of the
performance critical pieces of Lexer component, and handling of
identifier token will become very tricky. Second one does not look
more elegant as per compiler theory and requires heavy string search
and comparisons, but, avoids changes to Lexer component, and all
subsequent trickiness involved for handling identifier tokens.

I preferred second approach since Clang parser is recursive descent in
nature, mentioned the same in the proposal document, and proceeded
further since there was no any remark on my approach from any one.
But, now, based on few other code review comments, which in turn have
root in my original approach, I would like to open it again, discuss
it, and come to a common conclusion before proceeding further.


--
mahesha



On Sat, Oct 27, 2012 at 9:21 PM, Mahesha HS <[hidden email]> wrote:

> On Sat, Oct 27, 2012 at 2:48 PM, Chandler Carruth <[hidden email]> wrote:
>> On Sat, Oct 27, 2012 at 2:10 AM, Chandler Carruth <[hidden email]> wrote:
>>> On Fri, Oct 26, 2012 at 11:14 PM, Mahesha HS <[hidden email]> wrote:
>>>> Hi Dmitri,
>>>>
>>>> Thanks for the review. I have taken care of all your comments. With
>>>> respect to the use of std::lower_bound() instead of our own binary
>>>> search implementation, looks like, it is *not* straightforward as the
>>>> search algorithm actually need to operate on a map table which holds
>>>> the pair <StringRef, Enum Type>. However, I will re-think about it and
>>>> modify it if it is possible in some way.
>>>>
>>>> I will be committing first three patches as most of the review
>>>> comments are taken care for them. I will cross verify and make sure
>>>> that these patches will *strictly* adhere to CLANG/LLVM coding
>>>> standard before committing.
>>>
>>> I'm really sorry, but that is *not* the policy of pre-commit review,
>>> or commit access after *approval*.
>>>
>>> Please wait until you get explicit approval, especially for such very
>>> large feature additions to Clang. Reviewers often focus on one set of
>>> issues for a particular round of review and may have further issues
>>> they need you to address before committing.
>>
>> I see you have already committed all three without waiting for
>> approval. Please don't abuse the commit privilege in this way.
>>
>> The code still has some demonstrable problems by the way, which I
>> found with only a casual glance. I would assume that these would have
>> come up in subsequent review:
>>
>
> Hi Chandler,
>
>> 1) The over-use of OwningPtr seems like a serious code smell. I would
>> want to understand why these are pointers at all here.
>
> In general, any pragma support in Clang requires it to be first
> registered with Preprocessor. The way to register it is by deriving a
> new class from pragma base class interface - "class Pragma", and then
> by adding an instance of the derived class to Preprocessor's pragma
> handler object. Since OpenMP has around *fifteen* different pragma
> directives, we need a separate instance for each of them. So, there
> seems to be a over-use of OwningPtr.
>
> However, as I am new to Clang code base, my current usual practice is
> to look into exiting relevant code base and try to follow the same
> approach when it comes to allocating new objects, conservative
> implementation, etc. I looked into the Parser class, where already
> OwningPtr is used liberally to some extent to support few pragmas. So,
>  I thought of following the same approach.
>
> However, your comment is well taken. I will see if I can do it more
> efficient way. I also welcome your suggestions in this regard.
>
>> 2) There are copious "doxygen" formatted comments that don't use a
>> doxygen comment prefix of '///', instead using the normal '//'.
>
> I will be very careful with these violations.
>
>> 3) The test cases seem quite haphazard; these are often the last
>> things to be addressed in code review, but possibly I'm just skimming
>> the patch in too little detail and much of the testing will come
>> later.
>
> If possible, please give me some hints so that it helps me to create
> more robust test cases. This is very crucial at this point since I am
> still new to Clang environment.
>
>> 4) There is no -fno-openmp to complement -fopenmp. Having this ability
>> to turn off features by appending flags is something we strive for in
>> the driver.
>
> By default, OpenMP feature is *off* in all most all compilers. Only
> when the user passes -fopenmp, it will be turned *on*. So, I did not
> think about  -fno-openmp. However, I will  think about it, and support
> it too, if we decide that it will be useful.
>
> --
> mahesha



--
mahesha
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [cfe-commits] [LLVMdev] OpenMP support in CLANG: A proposal

Mahesha HS
I curiously looked into the OpenMP parsing in GCC. It follows the
method of string comparison but *without* maintaining any string table
in a following manner (by switching based on the first character of an
identifier token) . I think, I can also follow the same technique. I
should have done this long before in order to prevent unnecessarily
maintaining a string table. But, I still welcome other better
techniques, if any.

/* OpenMP 2.5 parsing routines.  */

/* Returns name of the next clause.
   If the clause is not recognized PRAGMA_OMP_CLAUSE_NONE is returned and
   the token is not consumed.  Otherwise appropriate pragma_omp_clause is
   returned and the token is consumed.  */

static pragma_omp_clause
c_parser_omp_clause_name (c_parser *parser)
{
  pragma_omp_clause result = PRAGMA_OMP_CLAUSE_NONE;

  if (c_parser_next_token_is_keyword (parser, RID_IF))
    result = PRAGMA_OMP_CLAUSE_IF;
  else if (c_parser_next_token_is_keyword (parser, RID_DEFAULT))
    result = PRAGMA_OMP_CLAUSE_DEFAULT;
  else if (c_parser_next_token_is (parser, CPP_NAME))
    {
      const char *p = IDENTIFIER_POINTER (c_parser_peek_token (parser)->value);

      switch (p[0])
        {
        case 'c':
          if (!strcmp ("copyin", p))
            result = PRAGMA_OMP_CLAUSE_COPYIN;
          else if (!strcmp ("copyprivate", p))
            result = PRAGMA_OMP_CLAUSE_COPYPRIVATE;
          break;
        case 'f':
          if (!strcmp ("firstprivate", p))
            result = PRAGMA_OMP_CLAUSE_FIRSTPRIVATE;
          break;
        case 'l':
          if (!strcmp ("lastprivate", p))
            result = PRAGMA_OMP_CLAUSE_LASTPRIVATE;
          break;
        case 'n':
          if (!strcmp ("nowait", p))
            result = PRAGMA_OMP_CLAUSE_NOWAIT;
          else if (!strcmp ("num_threads", p))
            result = PRAGMA_OMP_CLAUSE_NUM_THREADS;
          break;
        case 'o':
          if (!strcmp ("ordered", p))
            result = PRAGMA_OMP_CLAUSE_ORDERED;
          break;
        case 'p':
          if (!strcmp ("private", p))
            result = PRAGMA_OMP_CLAUSE_PRIVATE;
          break;
        case 'r':
          if (!strcmp ("reduction", p))
            result = PRAGMA_OMP_CLAUSE_REDUCTION;
          break;
        case 's':
          if (!strcmp ("schedule", p))
            result = PRAGMA_OMP_CLAUSE_SCHEDULE;
          else if (!strcmp ("shared", p))
            result = PRAGMA_OMP_CLAUSE_SHARED;
          break;
        }
    }

  if (result != PRAGMA_OMP_CLAUSE_NONE)
    c_parser_consume_token (parser);

  return result;
}

--
mahesha


On Sat, Oct 27, 2012 at 10:50 PM, Mahesha HS <[hidden email]> wrote:

> Dear All,
>
> Based on the few code review comments that I received recently, I do
> think that I should re-visit basic design that I had proposed
> initially, and I want to make sure that the community is in agreement
> with it, before I proceed further.
>
> The very first design question to be discussed is about
> *scanning/parsing* OpenMP names. As you know, there are so many OpenMP
> specific names like parallel, for, private, shared, etc. And,
> following are two possible choices to scan/parse them.
>
> 1.  Treat OpenMP names as *keywords*, introduce new lexical tokens for
> each one them, make changes to Lexer/Parser to scan them as either
> OpenMP keywords or as identifiers or as C/C++ keywords depending on
> the context.
>
> 2.  Store all OpenMP names in a string table, and while parsing OpenMP
> directive statements, perform string search and comparison in order to
> parse them as either valid OpenMP names (upon successful string
> search) or report syntax errors (upon unsuccessful string search).
>
> [Any other possible choices?]
>
> Both the above approaches have their pros and cons. First one looks
> elegant at least theoretically, but requires changes to some of the
> performance critical pieces of Lexer component, and handling of
> identifier token will become very tricky. Second one does not look
> more elegant as per compiler theory and requires heavy string search
> and comparisons, but, avoids changes to Lexer component, and all
> subsequent trickiness involved for handling identifier tokens.
>
> I preferred second approach since Clang parser is recursive descent in
> nature, mentioned the same in the proposal document, and proceeded
> further since there was no any remark on my approach from any one.
> But, now, based on few other code review comments, which in turn have
> root in my original approach, I would like to open it again, discuss
> it, and come to a common conclusion before proceeding further.
>
>
> --
> mahesha
>
>
>
> On Sat, Oct 27, 2012 at 9:21 PM, Mahesha HS <[hidden email]> wrote:
>> On Sat, Oct 27, 2012 at 2:48 PM, Chandler Carruth <[hidden email]> wrote:
>>> On Sat, Oct 27, 2012 at 2:10 AM, Chandler Carruth <[hidden email]> wrote:
>>>> On Fri, Oct 26, 2012 at 11:14 PM, Mahesha HS <[hidden email]> wrote:
>>>>> Hi Dmitri,
>>>>>
>>>>> Thanks for the review. I have taken care of all your comments. With
>>>>> respect to the use of std::lower_bound() instead of our own binary
>>>>> search implementation, looks like, it is *not* straightforward as the
>>>>> search algorithm actually need to operate on a map table which holds
>>>>> the pair <StringRef, Enum Type>. However, I will re-think about it and
>>>>> modify it if it is possible in some way.
>>>>>
>>>>> I will be committing first three patches as most of the review
>>>>> comments are taken care for them. I will cross verify and make sure
>>>>> that these patches will *strictly* adhere to CLANG/LLVM coding
>>>>> standard before committing.
>>>>
>>>> I'm really sorry, but that is *not* the policy of pre-commit review,
>>>> or commit access after *approval*.
>>>>
>>>> Please wait until you get explicit approval, especially for such very
>>>> large feature additions to Clang. Reviewers often focus on one set of
>>>> issues for a particular round of review and may have further issues
>>>> they need you to address before committing.
>>>
>>> I see you have already committed all three without waiting for
>>> approval. Please don't abuse the commit privilege in this way.
>>>
>>> The code still has some demonstrable problems by the way, which I
>>> found with only a casual glance. I would assume that these would have
>>> come up in subsequent review:
>>>
>>
>> Hi Chandler,
>>
>>> 1) The over-use of OwningPtr seems like a serious code smell. I would
>>> want to understand why these are pointers at all here.
>>
>> In general, any pragma support in Clang requires it to be first
>> registered with Preprocessor. The way to register it is by deriving a
>> new class from pragma base class interface - "class Pragma", and then
>> by adding an instance of the derived class to Preprocessor's pragma
>> handler object. Since OpenMP has around *fifteen* different pragma
>> directives, we need a separate instance for each of them. So, there
>> seems to be a over-use of OwningPtr.
>>
>> However, as I am new to Clang code base, my current usual practice is
>> to look into exiting relevant code base and try to follow the same
>> approach when it comes to allocating new objects, conservative
>> implementation, etc. I looked into the Parser class, where already
>> OwningPtr is used liberally to some extent to support few pragmas. So,
>>  I thought of following the same approach.
>>
>> However, your comment is well taken. I will see if I can do it more
>> efficient way. I also welcome your suggestions in this regard.
>>
>>> 2) There are copious "doxygen" formatted comments that don't use a
>>> doxygen comment prefix of '///', instead using the normal '//'.
>>
>> I will be very careful with these violations.
>>
>>> 3) The test cases seem quite haphazard; these are often the last
>>> things to be addressed in code review, but possibly I'm just skimming
>>> the patch in too little detail and much of the testing will come
>>> later.
>>
>> If possible, please give me some hints so that it helps me to create
>> more robust test cases. This is very crucial at this point since I am
>> still new to Clang environment.
>>
>>> 4) There is no -fno-openmp to complement -fopenmp. Having this ability
>>> to turn off features by appending flags is something we strive for in
>>> the driver.
>>
>> By default, OpenMP feature is *off* in all most all compilers. Only
>> when the user passes -fopenmp, it will be turned *on*. So, I did not
>> think about  -fno-openmp. However, I will  think about it, and support
>> it too, if we decide that it will be useful.
>>
>> --
>> mahesha
>
>
>
> --
> mahesha



--
mahesha
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [cfe-commits] [LLVMdev] OpenMP support in CLANG: A proposal

Dmitri Gribenko
On Sun, Oct 28, 2012 at 3:49 PM, Mahesha HS <[hidden email]> wrote:
> I curiously looked into the OpenMP parsing in GCC. It follows the
> method of string comparison but *without* maintaining any string table
> in a following manner (by switching based on the first character of an
> identifier token) . I think, I can also follow the same technique. I
> should have done this long before in order to prevent unnecessarily
> maintaining a string table. But, I still welcome other better
> techniques, if any.

Clang already does this sort of thing, but the matching code is
autogenerated by TableGen.  For example, take a look at
tools/clang/include/clang/AST/CommentCommandInfo.inc in the build
tree.

It might sound sensible to code the switch() for OMP clauses by hand
just because there are only a few of them.  But OTOH, it will create a
possibly unwanted precedent of hardcoding where we have the
infrastructure to autogenerate code.

Dmitri

--
main(i,j){for(i=2;;i++){for(j=2;j<i;j++){if(!(i%j)){j=0;break;}}if
(j){printf("%d\n",i);}}} /*Dmitri Gribenko <[hidden email]>*/
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [cfe-commits] [LLVMdev] OpenMP support in CLANG: A proposal

Chris Lattner

On Oct 28, 2012, at 6:54 AM, Dmitri Gribenko <[hidden email]> wrote:

> On Sun, Oct 28, 2012 at 3:49 PM, Mahesha HS <[hidden email]> wrote:
>> I curiously looked into the OpenMP parsing in GCC. It follows the
>> method of string comparison but *without* maintaining any string table
>> in a following manner (by switching based on the first character of an
>> identifier token) . I think, I can also follow the same technique. I
>> should have done this long before in order to prevent unnecessarily
>> maintaining a string table. But, I still welcome other better
>> techniques, if any.
>
> Clang already does this sort of thing, but the matching code is
> autogenerated by TableGen.  For example, take a look at
> tools/clang/include/clang/AST/CommentCommandInfo.inc in the build
> tree.
>
> It might sound sensible to code the switch() for OMP clauses by hand
> just because there are only a few of them.  But OTOH, it will create a
> possibly unwanted precedent of hardcoding where we have the
> infrastructure to autogenerate code.

Personally, I'd rather that you optimize for keeping the parsing code *simple*.  There are places in the parser that we have to optimize for performance, but they are rare, and OpenMP directives are not common enough to go to extreme measures like this.  Please just make the code as simple, clear, and short as possible.

-Chris
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [cfe-commits] [LLVMdev] OpenMP support in CLANG: A proposal

Mahesha HS
On Sun, Oct 28, 2012 at 11:38 PM, Chris Lattner <[hidden email]> wrote:

>
> On Oct 28, 2012, at 6:54 AM, Dmitri Gribenko <[hidden email]> wrote:
>
>> On Sun, Oct 28, 2012 at 3:49 PM, Mahesha HS <[hidden email]> wrote:
>>> I curiously looked into the OpenMP parsing in GCC. It follows the
>>> method of string comparison but *without* maintaining any string table
>>> in a following manner (by switching based on the first character of an
>>> identifier token) . I think, I can also follow the same technique. I
>>> should have done this long before in order to prevent unnecessarily
>>> maintaining a string table. But, I still welcome other better
>>> techniques, if any.
>>
>> Clang already does this sort of thing, but the matching code is
>> autogenerated by TableGen.  For example, take a look at
>> tools/clang/include/clang/AST/CommentCommandInfo.inc in the build
>> tree.
>>
>> It might sound sensible to code the switch() for OMP clauses by hand
>> just because there are only a few of them.  But OTOH, it will create a
>> possibly unwanted precedent of hardcoding where we have the
>> infrastructure to autogenerate code.
>
> Personally, I'd rather that you optimize for keeping the parsing code *simple*.  There are places in the parser that we have to optimize for performance, but they are rare, and OpenMP directives are not common enough to go to extreme measures like this.  Please just make the code as simple, clear, and short as possible.

I agree with you. I think, I am in a right direction, though some
refactoring is required to make the code further *simple*. I will make
sure that that code is clear, simple, and short as possible.

--
mahesha

>
> -Chris



--
mahesha

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: OpenMP support in CLANG: A proposal

Olaf Krzikalla
In reply to this post by Mahesha HS
Hi,

I did a quick look at your code and have some comments about it:

1. I don't think it is neccessary to introduce different classes for the
various sub-domains. One pragma handler class should do the trick. Also
I don't think that you need extra tokens. The sub-domain is always an
identifier. Parse it and schedule based on a string comparision.

2. Do not derive OpenMP directives from Stmt. Literally they are no
statements but directives. OpenMP directives attribute the statement
following immediately (and I use "attribute" here by full intent -
always keep C++11 attributes in mind). If there are also other pragmas
before a for-statement you might run in serious trouble. That said, you
should try to use (and maybe extend) the already installed attribute
framework in the AST.

3. I wonder if we need to touch Sema and Parser the way you do. Granted,
the Parser needs some extensions (more on that in the next bullet). But
the parsing should be done in the handler, since OpenMP is not a part of
C/C++ but rather an extension.

4. Last and most important point for other people still reading: clang
DOES need a way to parse expressions inside of pragmas. Admittedly this
violates the separation of concerns since pragmas exist at the
preprocessor level and expressions exist at the Sema level. However
that's just how it is. I hacked my own solution in Scout but after many
hours of bug fixing I won't advertise that solution here. Instead I
would opt for a parser redesign so that it is possible to have more than
one parser at the same time operating on a token stream. Then we could
construct a parser in a pragma handler and just call ParserExpression
(or what else needs to be parsed). Thus the state of the global parser
is not touched and it will happily jump to eol.


Best regards
Olaf
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: OpenMP support in CLANG: A proposal

Mahesha HS
On Tue, Nov 6, 2012 at 8:45 PM, Olaf Krzikalla
<[hidden email]> wrote:
> Hi,
>
> I did a quick look at your code and have some comments about it:

Thanks for the review Olaf. Yes, I have re-factored the code to a
greater extent to simplify the things, and I am still working on
re-factoring. However, please find my answers to some of your
questions which are inlined.

>
> 1. I don't think it is neccessary to introduce different classes for the
> various sub-domains. One pragma handler class should do the trick.

I already re-implemented it exactly as you mentioned. The initial
thinking of introducing different classes was based on the design
decision that "Pragma handler" should call appropriate handler based
on the omp directive name like "#pragma omp parallel", "#pragma omp
for", etc. So I landed up deriving a separate class for each omp
pragma directive name, from pragma handler interface.  Now, I
re-implemented it by having only one omp pragma handler class, and
parsing the omp directive names by making use of appropriate enums and
string comparison.

> Also I
> don't think that you need extra tokens.

If we handle everything within a Handler it-self, then, tokens may not
be required. However, OpenMP syntax and semantics is quite involved.
We have to ask global Parser and Sema to do it. Following are the
reasons for it.

1. OpenMP statements are part of function body statements.
2. OpenMP statements contain expression. scope (in case of "if"
clause) and structured blocks. So, we have to piggy back Global parser
to handle scopes, parse expressions and compound statement (structured
block). Moreover,  few OpenMP constructs like "barrier" are valid only
within a structured block (parallel region) which follows omp parallel
constructs. That means,  OpenMP parsing is recursive in nature.

So we need to have tokens which represent different OpenMP directives
(note - only for directives not for clauses), and Handler will insert
those tokens to token streams depending on the type of the omp
directive. Then, global Parser and Sema will take it forward.

> The sub-domain is always an
> identifier. Parse it and schedule based on a string comparision.

I agree. I am already re-implemented  exactly as you mentioned.

>
> 2. Do not derive OpenMP directives from Stmt. Literally they are no
> statements but directives. OpenMP directives attribute the statement
> following immediately (and I use "attribute" here by full intent - always
> keep C++11 attributes in mind). If there are also other pragmas before a
> for-statement you might run in serious trouble. That said, you should try to
> use (and maybe extend) the already installed attribute framework in the AST.

I accept. I will re-think about it, and re-implement it. Probably I
will share the AST design before implementing.

>
> 3. I wonder if we need to touch Sema and Parser the way you do. Granted, the
> Parser needs some extensions (more on that in the next bullet). But the
> parsing should be done in the handler, since OpenMP is not a part of C/C++
> but rather an extension.

As I mentioned above, I do think that global Parser and Sema need to
be involved in handling of OpenMP stuffs, at least for the time being,
as per the present Clang design. However, this topic is subjected to
further discussion.

--
mahesha

>
> 4. Last and most important point for other people still reading: clang DOES
> need a way to parse expressions inside of pragmas. Admittedly this violates
> the separation of concerns since pragmas exist at the preprocessor level and
> expressions exist at the Sema level. However that's just how it is. I hacked
> my own solution in Scout but after many hours of bug fixing I won't
> advertise that solution here. Instead I would opt for a parser redesign so
> that it is possible to have more than one parser at the same time operating
> on a token stream. Then we could construct a parser in a pragma handler and
> just call ParserExpression (or what else needs to be parsed). Thus the state
> of the global parser is not touched and it will happily jump to eol.
>
>
> Best regards
> Olaf
> _______________________________________________
> cfe-dev mailing list
> [hidden email]
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev



--
mahesha
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
12