[cfe-commits] [PATCH] First OpenMP patch

[Alexey Bataev]

Hello everybody,
 Are there any comments on this patch? Can it be committed?

Best regards,
 Alexey Bataev
 =============
 Software Engineer
 Intel Compiler Team
 Intel Corp.
----- Original Message -----
From: Alexey Bataev
Sent: 12/24/12 03:05 PM
To: Douglas Gregor, Alexey Bataev
Subject: Re: [cfe-commits] [PATCH] First OpenMP patch

Doug,
 I've fixed the patch. Please, take a look. 

 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 < a.bataev at gmx.com > 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 < a.bataev at gmx.com > 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 < gribozavr at gmail.com > wrote: > On Wed, Dec 19, 2012 at 10:53 PM, Douglas Gregor < dgregor at apple.com > wrote: >> >> On Dec 19, 2012, at 3:30 AM, Dmitri Gribenko < gribozavr at gmail.com > wrote: >> >>> On Wed, Dec 19, 2012 at 1:02 PM, Alexey Bataev < a.bataev at gmx.com > 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>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130109/06de104f/attachment.html>