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

[Eli Friedman]

On Sun, Oct 21, 2012 at 4:43 AM, Mahesha HS <mahesha.llvm at gmail.com> wrote:
> On Sat, Oct 20, 2012 at 4:38 PM, Mahesha HS <mahesha.llvm at gmail.com> wrote:
>> On Fri, Oct 19, 2012 at 4:03 AM, Eli Friedman <eli.friedman at gmail.com> wrote:
>>> On Thu, Oct 18, 2012 at 6:34 AM, Mahesha HS <mahesha.llvm at gmail.com> wrote:
>>>> 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.
>>>
>>> +//--------------------------------
>>> +// OpenMP directives
>>> +//--------------------------------
>>> +// \#pragam omp parallel ...
>>> +TOK(pragma_omp_parallel)
>>> +// \#pragam omp for ...
>>> +TOK(pragma_omp_for)
>>>
>>> Please use ANNOTATION(pragma_omp_parallel) etc. instead.
>>
>> I have taken care of it. Also, for patch 1 related test case, I
>> replaced -v by -###.
>>
>>>
>>> +/// PragmaOmpHandler - "\#pragma omp ...".
>>> +template<class T, tok::TokenKind TKind>
>>> +struct PragmaOmpHandler : public PragmaHandler {
>>> +  PragmaOmpHandler() : PragmaHandler(T::Name) {}
>>> +  virtual void HandlePragma(Preprocessor &PP, PragmaIntroducerKind Introducer,
>>> +                            Token &OmpTok) {
>>> +    PP.HandlePragmaOmp(OmpTok, TKind);
>>> +  }
>>> +};
>>>
>>> Something like following would make your patch substantially shorter:
>>>
>>> struct PragmaOmpHandler : public PragmaHandler {
>>>   tok::TokenKind TKind;
>>>   PragmaOmpHandler(tok::TokenKind Kind, StringRef Name)
>>>     : PragmaHandler(Name), TKind(Kind) {}
>>>   virtual void HandlePragma(Preprocessor &PP, PragmaIntroducerKind Introducer,
>>>                             Token &OmpTok) {
>>>     PP.HandlePragmaOmp(OmpTok, TKind);
>>>   }
>>> };
>>
>> My mis-understanding of PragmaNamespace::AddPragma() function, in
>> particular, mis-understanding of *assertion* statement within this
>> function mislead me.  I was in an impression that, adding same class
>> more than once to same pragma namespace will result in assertion
>> failure. But, I had not carefully looked into it that it should be
>> fine as long as the associated names are different. Anyway, I have
>> taken care of this too.
>>
>>>
>>>
>>> Please add a test that the pragmas round-trip correctly through clang -E.
>>
>> This test case is getting an assertion failure. Looks like, all the
>> #pragmas which requires parsing, has to be registered (to
>> preprocessor) from libParse.a just like how #pragma unused, etc are
>> handled? When -E option is added, these pragmas are handled by
>> Preprocessor as unknown pragmas.
>>
>> Waiting for more clarification from you.
>
> I have proceeded further, and followed exactly the similar approach as
> implemented in case of other pragmas like  #pragma unused, etc.
> Attached is the revised patch (omp_pragma_registration_revised.patch).
>
> In case of any further comments, please let me know. Otherwise, we
> need to check-in both patch 1 (fopenmp_option_support.patch) and patch
> 2 (this one).

You never sent the revised fopenmp_option_support.patch.

Index: include/clang/Lex/Preprocessor.h
===================================================================
--- include/clang/Lex/Preprocessor.h	(revision 166387)
+++ include/clang/Lex/Preprocessor.h	(working copy)
@@ -161,6 +161,13 @@
   /// \brief True if we are pre-expanding macro arguments.
   bool InMacroArgPreExpansion;

+  /// \brief When an OpenMP pragma is ignored, we emit a warning message saying
+  /// so, but only once per translation unit irrespective of the number of
+  /// OpenMP pragmas appeared in the translation unit. This flag keeps track of
+  /// whether the unkown pragma warning message is emitted or not for the
+  /// current translation unit.
+  bool PragmaOmpUnknownWarning;

Please move this into the parser, since you've moved everything else.
(You might need to store a reference to the flag in PragmaOmpHandler.)

Otherwise, looks fine.

-Eli