On Dec 24, 2012, at 3:05 AM, Alexey Bataev <[email protected]> wrote:
> Doug,
> I've fixed the patch. Please, take a look.
I went ahead and committed a tweaked version of this patch as r172509, thanks!
A few nits (that I've fixed):
Index: include/clang/Driver/Options.td
===================================================================
--- include/clang/Driver/Options.td (revision 169213)
+++ include/clang/Driver/Options.td (working copy)
@@ -598,7 +598,8 @@
def fobjc_sender_dependent_dispatch : Flag<["-"],
"fobjc-sender-dependent-dispatch">, Group<f_Group>;
def fobjc : Flag<["-"], "fobjc">, Group<f_Group>;
def fomit_frame_pointer : Flag<["-"], "fomit-frame-pointer">, Group<f_Group>;
-def fopenmp : Flag<["-"], "fopenmp">, Group<f_Group>;
+def fopenmp : Flag<["-"], "fopenmp">, Group<f_Group>, Flags<[CC1Option]>;
+def fno_openmp : Flag<["-"], "fno-openmp">, Group<f_Group>;
-fno-openmp is no longer used.
+ // OpenMP definition
+ if (LangOpts.OpenMP) {
+ // OpenMP 2.2: In implementations that supports a preprocessor, the
_OPENMP macro name
+ // is defined to have the decimal value yyyymm where yyyy and mm are the
year and
+ // the month designations of the version of the OpenMP API that the
implementation support.
+ Builder.defineMacro("_OPENMP", "201107");
+ }
Sources should always be wrapped at 80 columns.
- Doug
> Best regards,
>
> Alexey Bataev
> =============
> Software Engineer
> Intel Compiler Team
>
>
>> ----- Original Message -----
>> From: Douglas Gregor
>> Sent: 12/21/12 10:01 PM
>> To: Alexey Bataev
>> Subject: Re: [cfe-commits] [PATCH] First OpenMP patch
>>
>>
>> On Dec 20, 2012, at 8:47 PM, Alexey Bataev <[email protected]> wrote:
>>
>>> Hi Doug,
>>> Thank you for review. Here are my comments.
>>> 1. include/clang/Basic/DiagnosticDriverKinds.td (no changes) - Agree, my
>>> bad.
>>> 2. docs/OpenMP.rst - Ok, I'll remove it.
>>> 3. lib/Frontend/InitPreprocessor.cpp (comment) - Agree. As to version
>>> number, I think we can remove it. It seems the section numbers are stable.
>>
>> Okay.
>>
>>> 4. lib/Driver/Tools.cpp and include/clang/Basic/DiagnosticGroups.td
>>> (source-uses-openmp warning group) - Do we really need to remove this
>>> warning group? It is reserved for future use, I plan to emit a warning on
>>> the very first OpenMP pragma if no -fopenmp compiler option was specified.
>>> This warning (and maybe some others) is planned to be the part of this
>>> warning group, so the user could turn on/off the warning.
>>
>> This warning doesn't make sense until -fopenmp actually works. We should not
>> change the behavior of the compiler w.r.t. OpenMP for users until we flip
>> the switch to make -fopenmp turn on our complete, working implementation.
>>
>> Additionally, the only reason to add a placeholder diagnostic group is for
>> backward compatibility, so we recognize that warning group name on the
>> command line. For new warning groups, they should be added with the first
>> warning in that group.
>>
>>> 5. test/Driver/openmp-options.c - if warning group won't be removed, we
>>> need to keep this test. As to OpenMP/<new-test>, I'll add it.
>>
>> Okay. test/Derived/openmp-options.c can come back when it makes sense. We're
>> not there yet.
>>
>> - Doug
>>
>>> Best regards,
>>> Alexey Bataev
>>> =============
>>> Software Engineer
>>> Intel Compiler Team
>>>
>>>
>>>> ----- Original Message -----
>>>> From: Douglas Gregor
>>>> Sent: 12/20/12 11:40 PM
>>>> To: Alexey Bataev
>>>> Subject: Re: [cfe-commits] [PATCH] First OpenMP patch
>>>>
>>>>
>>>> On Dec 20, 2012, at 1:43 AM, Alexey Bataev <[email protected]> wrote:
>>>>
>>>>> Hello guys,
>>>>> I agree with all your comments. So I fixed the initial patch. See attach.
>>>>
>>>> Thanks. A few more comments:
>>>>
>>>> Index: include/clang/Basic/DiagnosticDriverKinds.td
>>>> ===================================================================
>>>> --- include/clang/Basic/DiagnosticDriverKinds.td (revision 169213)
>>>> +++ include/clang/Basic/DiagnosticDriverKinds.td (working copy)
>>>> @@ -146,4 +146,4 @@
>>>> "analyzer-config option '%0' has a key but no value">;
>>>> def err_analyzer_config_multiple_values : Error<
>>>> "analyzer-config option '%0' should contain only one '='">;
>>>> -}
>>>> +}
>>>>
>>>> This isn't actually changing anything.
>>>>
>>>> Index: docs/OpenMP.rst
>>>> ===================================================================
>>>> --- docs/OpenMP.rst (revision 0)
>>>> +++ docs/OpenMP.rst (revision 0)
>>>> @@ -0,0 +1,21 @@
>>>>
>>>> We don't need this now.
>>>>
>>>> Index: lib/Frontend/InitPreprocessor.cpp
>>>> ===================================================================
>>>> --- lib/Frontend/InitPreprocessor.cpp (revision 169213)
>>>> +++ lib/Frontend/InitPreprocessor.cpp (working copy)
>>>> @@ -641,6 +641,13 @@
>>>> "__attribute__((objc_ownership(none)))");
>>>> }
>>>>
>>>> + // OpenMP definition
>>>> + if (LangOpts.OpenMP) {
>>>> + // Define _OPENMP macro with value 201107 for OpenMP v. 3.1
>>>> + // according to p. 2.2 of OpenMP API specification
>>>> + Builder.defineMacro("_OPENMP", "201107");
>>>> + }
>>>> +
>>>>
>>>> We're going to end up with a lot of OpenCP specification citations, so
>>>> let's settle on a form now. I suggest:
>>>>
>>>> // OpenMPv3.1 2.2:
>>>> // In implementations that support a preprocessor, the _OPENMP macro
>>>> name is defined to have
>>>> // the decimal value yyyymm where yyyy and mm are the year and month
>>>> designations of the
>>>> // version of the OpenMP API that the implementation supports.
>>>>
>>>> which is similar to what we do for C/C++. If section numbers tend to be
>>>> stable, we can drop the v3.1. I don't know OpenMP well enough to guess.
>>>>
>>>> Index: lib/Driver/Tools.cpp
>>>> ===================================================================
>>>> --- lib/Driver/Tools.cpp (revision 169213)
>>>> +++ lib/Driver/Tools.cpp (working copy)
>>>> @@ -2510,6 +2510,15 @@
>>>>
>>>> Args.AddLastArg(CmdArgs, options::OPT_pthread);
>>>>
>>>> + // When both -fopenmp and -fno-openmp options are present,
>>>> + // later one wins.
>>>> + if (Arg *A = Args.getLastArg(options::OPT_fopenmp,
>>>> + options::OPT_fno_openmp)) {
>>>> + // Do not pass -fopenmp from driver to frontend for now
>>>> + if (A->getOption().matches(options::OPT_fno_openmp)) {
>>>> + CmdArgs.push_back("-Wno-source-uses-openmp");
>>>> + }
>>>> + }
>>>>
>>>> // -stack-protector=0 is default.
>>>> unsigned StackProtectorLevel = 0;
>>>>
>>>> Since there are no longer any warnings under this diagnostic group, this
>>>> code should be removed.
>>>>
>>>>
>>>> Index: include/clang/Basic/DiagnosticGroups.td
>>>> ===================================================================
>>>> --- include/clang/Basic/DiagnosticGroups.td (revision 169213)
>>>> +++ include/clang/Basic/DiagnosticGroups.td (working copy)
>>>> @@ -407,6 +407,9 @@
>>>> ThreadSafetyAnalysis,
>>>> ThreadSafetyPrecise]>;
>>>>
>>>> +// OpenMP warnings
>>>> +def SourceUsesOpenMP : DiagGroup<"source-uses-openmp">;
>>>> +
>>>> // Note that putting warnings in -Wall will not disable them by default.
>>>> If a
>>>> // warning should be active _only_ when -Wall is passed in, mark it as
>>>> // DefaultIgnore in addition to putting it here.
>>>>
>>>> We don't need this diagnostic group now.
>>>>
>>>> Index: test/Driver/openmp-options.c
>>>> ===================================================================
>>>> --- test/Driver/openmp-options.c (revision 0)
>>>> +++ test/Driver/openmp-options.c (revision 0)
>>>>
>>>> I think this test should go away for now, since we don't want to have
>>>> -Wno-source-uses-openmp.
>>>>
>>>> Instead, we should have a test in a new test/OpenMP directory that runs
>>>> clang -cc1 -fopenmp and then uses an #if/#error to make sure _OPENMP is
>>>> being set correctly.
>>>>
>>>> - Doug
>>>>
>>>>> Best regards,
>>>>> Alexey.
>>>>>
>>>>>
>>>>>> ----- Original Message -----
>>>>>> From: Richard Smith
>>>>>> Sent: 12/20/12 01:12 AM
>>>>>> To: Dmitri Gribenko
>>>>>> Subject: Re: [cfe-commits] [PATCH] First OpenMP patch
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> On Wed, Dec 19, 2012 at 1:09 PM, Dmitri Gribenko <[email protected]>
>>>>>> wrote:
>>>>>> > On Wed, Dec 19, 2012 at 10:53 PM, Douglas Gregor <[email protected]>
>>>>>> > wrote:
>>>>>> >>
>>>>>> >> On Dec 19, 2012, at 3:30 AM, Dmitri Gribenko <[email protected]>
>>>>>> >> wrote:
>>>>>> >>
>>>>>> >>> On Wed, Dec 19, 2012 at 1:02 PM, Alexey Bataev <[email protected]>
>>>>>> >>> wrote:
>>>>>> >>>> Dmitry, thanks for a good advice about :option:. Fixed patch is in
>>>>>> >>>> the
>>>>>> >>>> attach.
>>>>>> >>>
>>>>>> >>> LGTM. I think that there is a consensus about command line flags
>>>>>> >>> and
>>>>>> >>> warnings, so this can go in.
>>>>>> >>
>>>>>> >> Sorry to come late to the party, but I think this is the wrong
>>>>>> >> approach for our users. Our current behavior is that we simply drop
>>>>>> >> "-fopenmp". If we change that behavior to
>>>>>> >>
>>>>>> >> warning: Experimental OpenMP support enabled
>>>>>> >>
>>>>>> >> how have we helped the user? At best, it does nothing and they ignore
>>>>>> >> the warning. At worst, it opens up a ton of questions: should I
>>>>>> >> depend on the experimental implementation? What does it actually do?
>>>>>> >> What was it doing before? How do I shut this warning off? This is
>>>>>> >> especially important to consider for options like -fopenmp that
>>>>>> >> currently exist for GNU compatibility. Changing the behavior in any
>>>>>> >> way other than actually implementing that functionality is not a net
>>>>>> >> win for users.
>>>>>> >>
>>>>>> >> All we need for OpenMP at this stage is a new LangOpt for OpenMP
>>>>>> >> (which is part of the patch) and a -cc1 option "-fopenmp" that turns
>>>>>> >> on that LangOpt. It also makes sense to define _OPENMP when that
>>>>>> >> LangOpt is true, so that we can write a test for -fopenmp.
>>>>>> >>
>>>>>> >> It is very important that "-fopenmp" still be dropped by the driver.
>>>>>> >> The -fopenmp at the -cc1 level will be separate, and used for our
>>>>>> >> testing. Once OpenMP is actually usable, we can wire the driver's
>>>>>> >> -fopenmp up to the -cc1 -fopenmp.
>>>>>> >
>>>>>> > That's a good point. But in order to test the implementation on some
>>>>>> > real OpenMP programs easily, we might still want to introduce a
>>>>>> > frontend option, just a different one -- like '-fexperimental-openmp'.
>>>>>> > We can also drop the warning then.
>>>>>>
>>>>>> For testing, we can use -Xclang -fopenmp
>>>>>
>>>>>
>>>>> <patch.p>
>>>
>
>
> <patch.p>
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
