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>
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
