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

[Mahesha HS]

Sorry, in my previous mail,  I had missed to attach changes to
"clang/include/clang/Basic/TokenKinds.def" in the patch 2. Please
refer to the patch (2) attached in *this* mail, instead of the one
sent in the previous mail. Patch 1 is fine.

--
mahesha


On Thu, Oct 18, 2012 at 5:02 PM, Mahesha HS <mahesha.llvm at gmail.com> wrote:
> Hi Eli,
>
> Thanks for all your comments.
>
> I have taken care of all your review comments. Yes, after I gone
> through your review
> comments, I also came to the conclusion that the addition of a new
> class for OpenMP
> pragma handling (class PragmaOmpHandler) is not necessarily required.
> However, initially, when  started, I my-self had an opinion that this
> class may required.
> Now, I removed this class, and moved the omp pragma registration directly into
> Preprocessor class.
>
> I have attached following two patches along with this mail. The
> patches also contains
>  relevant *test cases*.
>
> patch 1:  fopenmp_option_support.patch
> patch 2:  omp_pragma_registration.patch
>
> In patch 1, I have taken care of all your earlier review comments
> related to -fopenmp option
> support. Patch 2 contains the implementation for the omp pragma
> registration with
> Preprocessor.
>
> Following files are changed respectively.
>
> patch 1:
> ------------
> #.  clang/include/clang/Driver/Options.td
> #.  clang/include/clang/Basic/LangOptions.def
> #.  clang/lib/Driver/Tools.cpp
> #.  clang/lib/Frontend/CompilerInvocation.cpp
> #.  clang/test/Driver/clang_fopenmp_opt.c
>
> patch 2:
> ------------
> #.  clang/include/clang/Lex/Preprocessor.h
> #.  clang/lib/Lex/Preprocessor.cpp
> #.  clang/lib/Lex/Pragma.cpp
> #.  clang/test/Preprocessor/pragma_omp_ignored_warning.c
>
> --
> mahesha
>
>
> On Wed, Oct 17, 2012 at 7:00 AM, Eli Friedman <eli.friedman at gmail.com> wrote:
>> On Tue, Oct 16, 2012 at 4:11 AM, Mahesha HS <mahesha.llvm at gmail.com> 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
>
>
>
> --
> mahesha



-- 
mahesha
-------------- next part --------------
A non-text attachment was scrubbed...
Name: omp_pragma_registration.patch
Type: application/octet-stream
Size: 10929 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20121018/7aa3dc77/attachment.obj>