[cfe-commits] [LLVMdev] [cfe-dev] OpenMP support in CLANG: A proposal
mahesha.llvm at gmail.com
Thu Oct 18 06:34:41 PDT 2012
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.
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
> 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
> 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
>>> 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
>> 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.
-------------- next part --------------
A non-text attachment was scrubbed...
Size: 10929 bytes
Desc: not available
More information about the cfe-commits