<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 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.<br></span></span></blockquote><div><br></div>Thanks. A few more comments:</div><div><br></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><br></div><div>This isn't actually changing anything.</div><div><br></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><br></div><div>We don't need this now.</div></div></div><div><div><br></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><br></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><br></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><br></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><font face="TimesNewRoman"><span style="font-size: 13px;"><br></span></font></div><div><font face="TimesNewRoman"><span style="font-size: 13px;"><div>Index: lib/Driver/Tools.cpp</div><div>===================================================================</div><div>--- lib/Driver/Tools.cpp<span class="Apple-tab-span" style="white-space:pre"> </span>(revision 169213)</div><div>+++ lib/Driver/Tools.cpp<span class="Apple-tab-span" style="white-space:pre"> </span>(working copy)</div><div>@@ -2510,6 +2510,15 @@</div><div> </div><div> Args.AddLastArg(CmdArgs, options::OPT_pthread);</div><div> </div><div>+ // When both -fopenmp and -fno-openmp options are present,</div><div>+ // later one wins.</div><div>+ if (Arg *A = Args.getLastArg(options::OPT_fopenmp,</div><div>+ options::OPT_fno_openmp)) {</div><div>+ // Do not pass -fopenmp from driver to frontend for now</div><div>+ if (A->getOption().matches(options::OPT_fno_openmp)) {</div><div>+ CmdArgs.push_back("-Wno-source-uses-openmp");</div><div>+ }</div><div>+ }</div><div> </div><div> // -stack-protector=0 is default.</div><div> unsigned StackProtectorLevel = 0;</div><div><br></div><div>Since there are no longer any warnings under this diagnostic group, this code should be removed.</div><div><br></div></span></font></div><div><br></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><br></div><div>We don't need this diagnostic group now.</div><div><br></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><br></div><div>I think this test should go away for now, since we don't want to have -Wno-source-uses-openmp.</div><div><br></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><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.<br>
<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: Richard Smith</span></span></div><div style="margin: 0px; padding: 0px; ">
<span style="font-family:Verdana"><span style="font-size:12px">Sent: 12/20/12 01:12 AM</span></span></div><div style="margin: 0px; padding: 0px; ">
<span style="font-family:Verdana"><span style="font-size:12px">To: Dmitri Gribenko</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>
<div>
<pre style="white-space: pre-wrap; word-wrap: break-word; font-size:11;pre">
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</pre>
</div>
</div>
</blockquote><p style="margin:0px; padding:0px;">
</p>
<br>
</span></span>
<span><patch.p></span></blockquote></div><br></body></html>