<span style='font-family:Verdana'><span style='font-size:12px'>Hi Doug,<br />
Thank you for review. Here are my comments.<br />
1. include/clang/Basic/DiagnosticDriverKinds.td (no changes) - Agree, my bad.<br />
2. docs/OpenMP.rst - Ok, I'll remove it.<br />
3. lib/Frontend/InitPreprocessor.cpp (comment) - Agree. As to version number, I think we can remove it. It seems the section numbers are stable.<br />
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.<br />
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.<br />
<br />
Best regards,<br />
Alexey Bataev<br />
=============
<div>
Software Engineer</div>
<div>
Intel Compiler Team</div>
<br />
<p style="margin:0px; padding:0px;" >
</p>
<blockquote style="border-left: 1px solid #CCC; padding-left: 5px; margin-left: 5px; margin-bottom: 0px; margin-top: 0px; margin-right: 0px;" type="cite">
<p style="margin:0px; padding:0px;" >
<span style="font-family:Verdana"><span style="font-size:12px">----- Original Message -----</span></span></p>
<p style="margin:0px; padding:0px;" >
<span style="font-family:Verdana"><span style="font-size:12px">From: Douglas Gregor</span></span></p>
<p style="margin:0px; padding:0px;" >
<span style="font-family:Verdana"><span style="font-size:12px">Sent: 12/20/12 11:40 PM</span></span></p>
<p style="margin:0px; padding:0px;" >
<span style="font-family:Verdana"><span style="font-size:12px">To: Alexey Bataev</span></span></p>
<p style="margin:0px; padding:0px;" >
<span style="font-family:Verdana"><span style="font-size:12px">Subject: Re: [cfe-commits] [PATCH] First OpenMP patch</span></span></p>
<br />
<div>
<br />
<div>
<div>
On Dec 20, 2012, at 1:43 AM, Alexey Bataev <<a href="mailto:a.bataev@gmx.com">a.bataev@gmx.com</a>> wrote:</div>
<br class="Apple-interchange-newline" />
<blockquote type="cite">
<span style="font-family:Verdana"><span style="font-size:12px">Hello guys,<br />
I agree with all your comments. So I fixed the initial patch. See attach.</span></span></blockquote>
<div>
</div>
Thanks. A few more comments:</div>
<div>
</div>
<div>
<div>
Index: include/clang/Basic/DiagnosticDriverKinds.td</div>
<div>
===================================================================</div>
<div>
--- include/clang/Basic/DiagnosticDriverKinds.td<span class="Apple-tab-span" style="white-space:pre"> </span>(revision 169213)</div>
<div>
+++ include/clang/Basic/DiagnosticDriverKinds.td<span class="Apple-tab-span" style="white-space:pre"> </span>(working copy)</div>
<div>
@@ -146,4 +146,4 @@</div>
<div>
"analyzer-config option '%0' has a key but no value">;</div>
<div>
def err_analyzer_config_multiple_values : Error<</div>
<div>
"analyzer-config option '%0' should contain only one '='">;</div>
<div>
-}</div>
<div>
+}</div>
<div>
</div>
<div>
This isn't actually changing anything.</div>
<div>
</div>
<div>
<div>
Index: docs/OpenMP.rst</div>
<div>
===================================================================</div>
<div>
--- docs/OpenMP.rst<span class="Apple-tab-span" style="white-space:pre"> </span>(revision 0)</div>
<div>
+++ docs/OpenMP.rst<span class="Apple-tab-span" style="white-space:pre"> </span>(revision 0)</div>
<div>
@@ -0,0 +1,21 @@</div>
<div>
</div>
<div>
We don't need this now.</div>
</div>
</div>
<div>
<div>
</div>
<div>
<div>
Index: lib/Frontend/InitPreprocessor.cpp</div>
<div>
===================================================================</div>
<div>
--- lib/Frontend/InitPreprocessor.cpp<span class="Apple-tab-span" style="white-space:pre"> </span>(revision 169213)</div>
<div>
+++ lib/Frontend/InitPreprocessor.cpp<span class="Apple-tab-span" style="white-space:pre"> </span>(working copy)</div>
<div>
@@ -641,6 +641,13 @@</div>
<div>
"__attribute__((objc_ownership(none)))");</div>
<div>
}</div>
<div>
</div>
<div>
+ // OpenMP definition</div>
<div>
+ if (LangOpts.OpenMP) {</div>
<div>
+ // Define _OPENMP macro with value 201107 for OpenMP v. 3.1 </div>
<div>
+ // according to p. 2.2 of OpenMP API specification</div>
<div>
+ Builder.defineMacro("_OPENMP", "201107");</div>
<div>
+ }</div>
<div>
+</div>
<div>
</div>
<div>
We're going to end up with a lot of OpenCP specification citations, so let's settle on a form now. I suggest:</div>
<div>
</div>
<div>
// OpenMPv3.1 2.2:</div>
<div>
// <span style="font-size: 10pt; font-family: TimesNewRoman; ">In implementations that support a preprocessor, the </span><span style="font-size: 10pt; font-family: Courier; font-weight: 700; ">_OPENMP </span><span style="font-size: 10pt; font-family: TimesNewRoman; ">macro name is defined to have </span></div>
<div>
<span style="font-size: 10pt; font-family: TimesNewRoman; ">// the decimal value </span><span style="font-size: 10pt; font-family: 'TimesNewRoman,Italic'; ">yyyymm </span><span style="font-size: 10pt; font-family: TimesNewRoman; ">where </span><span style="font-size: 10pt; font-family: 'TimesNewRoman,Italic'; ">yyyy </span><span style="font-size: 10pt; font-family: TimesNewRoman; ">and </span><span style="font-size: 10pt; font-family: 'TimesNewRoman,Italic'; ">mm </span><span style="font-size: 10pt; font-family: TimesNewRoman; ">are the year and month designations of the </span></div>
<div>
<span style="font-size: 10pt; font-family: TimesNewRoman; ">// version of the OpenMP API that the implementation supports.</span></div>
<div>
</div>
<div>
<font face="TimesNewRoman"><span style="font-size: 13px;">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.</span></font></div>
<div>
</div>
<div>
<div>
<font face="TimesNewRoman"><span style="font-size: 13px;">Index: lib/Driver/Tools.cpp</span></font></div>
<div>
<font face="TimesNewRoman"><span style="font-size: 13px;">===================================================================</span></font></div>
<div>
<font face="TimesNewRoman"><span style="font-size: 13px;">--- lib/Driver/Tools.cpp<span class="Apple-tab-span" style="white-space:pre"> </span>(revision 169213)</span></font></div>
<div>
<font face="TimesNewRoman"><span style="font-size: 13px;">+++ lib/Driver/Tools.cpp<span class="Apple-tab-span" style="white-space:pre"> </span>(working copy)</span></font></div>
<div>
<font face="TimesNewRoman"><span style="font-size: 13px;">@@ -2510,6 +2510,15 @@</span></font></div>
<div>
<font face="TimesNewRoman"><span style="font-size: 13px;"> </span></font></div>
<div>
<font face="TimesNewRoman"><span style="font-size: 13px;"> Args.AddLastArg(CmdArgs, options::OPT_pthread);</span></font></div>
<div>
<font face="TimesNewRoman"><span style="font-size: 13px;"> </span></font></div>
<div>
<font face="TimesNewRoman"><span style="font-size: 13px;">+ // When both -fopenmp and -fno-openmp options are present,</span></font></div>
<div>
<font face="TimesNewRoman"><span style="font-size: 13px;">+ // later one wins.</span></font></div>
<div>
<font face="TimesNewRoman"><span style="font-size: 13px;">+ if (Arg *A = Args.getLastArg(options::OPT_fopenmp,</span></font></div>
<div>
<font face="TimesNewRoman"><span style="font-size: 13px;">+ options::OPT_fno_openmp)) {</span></font></div>
<div>
<font face="TimesNewRoman"><span style="font-size: 13px;">+ // Do not pass -fopenmp from driver to frontend for now</span></font></div>
<div>
<font face="TimesNewRoman"><span style="font-size: 13px;">+ if (A->getOption().matches(options::OPT_fno_openmp)) {</span></font></div>
<div>
<font face="TimesNewRoman"><span style="font-size: 13px;">+ CmdArgs.push_back("-Wno-source-uses-openmp");</span></font></div>
<div>
<font face="TimesNewRoman"><span style="font-size: 13px;">+ }</span></font></div>
<div>
<font face="TimesNewRoman"><span style="font-size: 13px;">+ }</span></font></div>
<div>
<font face="TimesNewRoman"><span style="font-size: 13px;"> </span></font></div>
<div>
<font face="TimesNewRoman"><span style="font-size: 13px;"> // -stack-protector=0 is default.</span></font></div>
<div>
<font face="TimesNewRoman"><span style="font-size: 13px;"> unsigned StackProtectorLevel = 0;</span></font></div>
<div>
</div>
<div>
<font face="TimesNewRoman"><span style="font-size: 13px;">Since there are no longer any warnings under this diagnostic group, this code should be removed.</span></font></div>
<div>
</div>
</div>
<div>
</div>
</div>
<div>
<div>
Index: include/clang/Basic/DiagnosticGroups.td</div>
<div>
===================================================================</div>
<div>
--- include/clang/Basic/DiagnosticGroups.td<span class="Apple-tab-span" style="white-space:pre"> </span>(revision 169213)</div>
<div>
+++ include/clang/Basic/DiagnosticGroups.td<span class="Apple-tab-span" style="white-space:pre"> </span>(working copy)</div>
<div>
@@ -407,6 +407,9 @@</div>
<div>
ThreadSafetyAnalysis,</div>
<div>
ThreadSafetyPrecise]>;</div>
<div>
</div>
<div>
+// OpenMP warnings</div>
<div>
+def SourceUsesOpenMP : DiagGroup<"source-uses-openmp">;</div>
<div>
+</div>
<div>
// Note that putting warnings in -Wall will not disable them by default. If a</div>
<div>
// warning should be active _only_ when -Wall is passed in, mark it as</div>
<div>
// DefaultIgnore in addition to putting it here.</div>
<div>
</div>
<div>
We don't need this diagnostic group now.</div>
<div>
</div>
<div>
<div>
Index: test/Driver/openmp-options.c</div>
<div>
===================================================================</div>
<div>
--- test/Driver/openmp-options.c<span class="Apple-tab-span" style="white-space: pre; "> </span>(revision 0)</div>
<div>
+++ test/Driver/openmp-options.c<span class="Apple-tab-span" style="white-space: pre; "> </span>(revision 0)</div>
<div>
</div>
<div>
I think this test should go away for now, since we don't want to have -Wno-source-uses-openmp.</div>
<div>
</div>
<div>
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.</div>
</div>
</div>
</div>
<div>
</div>
<div>
- Doug</div>
<div>
<br />
<blockquote type="cite">
<span style="font-family:Verdana"><span style="font-size:12px">Best regards,<br />
Alexey.</span></span><br />
<br />
<p style="margin:0px; padding:0px; margin:0px; padding:0px;">
<span style="font-family:Verdana"><span style="font-size:12px"> </span></span></p>
<blockquote style="border-left: 1px solid #CCC; padding-left: 5px; margin-left: 5px; margin-bottom: 0px; margin-top: 0px; margin-right: 0px;" type="cite">
<div style="margin: 0px; padding: 0px; ">
<span style="font-family:Verdana"><span style="font-size:12px"><span style="font-family:Verdana"><span style="font-size:12px">----- Original Message -----</span></span> </span></span></div>
<div style="margin: 0px; padding: 0px; ">
<span style="font-family:Verdana"><span style="font-size:12px"><span style="font-family:Verdana"><span style="font-size:12px">From: Richard Smith</span></span> </span></span></div>
<div style="margin: 0px; padding: 0px; ">
<span style="font-family:Verdana"><span style="font-size:12px"><span style="font-family:Verdana"><span style="font-size:12px">Sent: 12/20/12 01:12 AM</span></span> </span></span></div>
<div style="margin: 0px; padding: 0px; ">
<span style="font-family:Verdana"><span style="font-size:12px"><span style="font-family:Verdana"><span style="font-size:12px">To: Dmitri Gribenko</span></span> </span></span></div>
<div style="margin: 0px; padding: 0px; ">
<span style="font-family:Verdana"><span style="font-size:12px"><span style="font-family:Verdana"><span style="font-size:12px">Subject: Re: [cfe-commits] [PATCH] First OpenMP patch</span></span> </span></span></div>
<br />
<div>
<div>
<pre style="white-space: pre-wrap; word-wrap: break-word; font-size:11;white-space: pre-wrap; word-wrap: break-word; font-size:11;pre">
<span style="font-family:Verdana"><span style="font-size:12px">
On Wed, Dec 19, 2012 at 1:09 PM, Dmitri Gribenko <<a href="mailto:gribozavr@gmail.com">gribozavr@gmail.com</a>> wrote:
> On Wed, Dec 19, 2012 at 10:53 PM, Douglas Gregor <<a href="mailto:dgregor@apple.com">dgregor@apple.com</a>> wrote:
>>
>> On Dec 19, 2012, at 3:30 AM, Dmitri Gribenko <<a href="mailto:gribozavr@gmail.com">gribozavr@gmail.com</a>> wrote:
>>
>>> On Wed, Dec 19, 2012 at 1:02 PM, Alexey Bataev <<a href="mailto:a.bataev@gmx.com">a.bataev@gmx.com</a>> 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</span></span></pre>
</div>
</div>
</blockquote>
<p style="margin:0px; padding:0px; margin:0px; padding:0px;">
<span style="font-family:Verdana"><span style="font-size:12px"> </span></span></p>
<br />
<span><patch.p></span></blockquote>
</div>
</div>
</blockquote>
<p style="margin:0px; padding:0px;" >
</p>
<br />
</span></span>