<html><head><meta http-equiv="Content-Type" content="text/html charset=us-ascii"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><br><div><div>On Dec 20, 2012, at 8:47 PM, 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">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></span></span></blockquote><div><br></div>Okay.</div><div><br><blockquote type="cite"><span style="font-family:Verdana"><span style="font-size:12px">
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></span></span></blockquote><div><br></div><div>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.</div><div><br></div><div>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.</div><br><blockquote type="cite"><span style="font-family:Verdana"><span style="font-size:12px">
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></span></span></blockquote><div><br></div>Okay. test/Derived/openmp-options.c can come back when it makes sense. We're not there yet.</div><div><br></div><div><span class="Apple-tab-span" style="white-space:pre"> </span>- Doug</div><div><br><blockquote type="cite"><span style="font-family:Verdana"><span style="font-size:12px">
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"><div style="margin: 0px; padding: 0px; ">
<span style="font-family:Verdana"><span style="font-size:12px">----- Original Message -----</span></span></div><div style="margin: 0px; padding: 0px; ">
<span style="font-family:Verdana"><span style="font-size:12px">From: Douglas Gregor</span></span></div><div style="margin: 0px; padding: 0px; ">
<span style="font-family:Verdana"><span style="font-size:12px">Sent: 12/20/12 11:40 PM</span></span></div><div style="margin: 0px; padding: 0px; ">
<span style="font-family:Verdana"><span style="font-size:12px">To: Alexey Bataev</span></span></div><div 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></div>
<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>
</blockquote></div><br></body></html>