[cfe-dev] [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 <mahesha.llvm at gmail.com> wrote:
> On Sat, Oct 27, 2012 at 2:48 PM, Chandler Carruth <chandlerc at google.com> wrote:
>> On Sat, Oct 27, 2012 at 2:10 AM, Chandler Carruth <chandlerc at google.com> wrote:
>>> On Fri, Oct 26, 2012 at 11:14 PM, Mahesha HS <mahesha.llvm at gmail.com> 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