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. 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. 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.
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 d! rop 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
