[cfe-commits] [LLVMdev] [cfe-dev] 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 <mahesha.llvm at gmail.com> 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 <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



-- 
mahesha