<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Feb 12, 2018 at 4:01 PM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">ah, sweet :)</div><div class="HOEnZb"><div class="h5"><br><div class="gmail_quote"><div dir="ltr">On Mon, Feb 12, 2018 at 2:59 PM Eric Fiselier <<a href="mailto:eric@efcs.ca" target="_blank">eric@efcs.ca</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Mon, Feb 12, 2018 at 3:35 PM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><br><br><div class="gmail_quote"><span><div dir="ltr">On Mon, Feb 12, 2018 at 2:25 PM Eric Fiselier <<a href="mailto:eric@efcs.ca" target="_blank">eric@efcs.ca</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Mon, Feb 12, 2018 at 9:15 AM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><br><br><div class="gmail_quote"><span><div dir="ltr">On Wed, Feb 7, 2018 at 10:38 AM Eric Fiselier via cfe-commits <<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Author: ericwf<br>
Date: Wed Feb 7 10:36:51 2018<br>
New Revision: 324498<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=324498&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project?rev=324498&view=rev</a><br>
Log:<br>
[Driver] Add option to manually control discarding value names in LLVM IR.<br>
<br>
Summary:<br>
Currently, assertion-disabled Clang builds emit value names when generating LLVM IR. This is controlled by the `NDEBUG` macro, and is not easily overridable. In order to get IR output containing names from a release build of Clang, the user must manually construct the CC1 invocation w/o the `-discard-value-names` option. This is less than ideal.<br>
<br>
For example, Godbolt uses a release build of Clang, and so when asked to emit LLVM IR the result lacks names, making it harder to read. Manually invoking CC1 on Compiler Explorer is not feasible.<br></blockquote></span><div><br>It wouldn't necessarily have to invoke CC1, it could use "-Xclang -discard-value-names".<br></div></div></div></blockquote><div><br></div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>If you were using an assertion build, and wanted to disable value names, then yes -- that would work. However it's the opposite case that is of interest:</div><div>When you're using a non-assertion build and want to keep value names. In that case invoking CC1 directly is required; otherwise the driver would pass</div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>"-discard-value-names".</div></div></div></div></blockquote></span><div><br>Ah, thanks for explaining!<br> </div><div><div class="m_3385180945354387933m_-6294943521944150138h5"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div> </div><div><div class="m_3385180945354387933m_-6294943521944150138m_-5693608131529091535m_3260618038478398407h5"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
This patch adds the driver options `-fdiscard-value-names` and `-fno-discard-value-names` which allow the user to override the default behavior. If neither is specified, the old behavior remains.<br>
<br>
Reviewers: erichkeane, aaron.ballman, lebedev.ri<br>
<br>
Reviewed By: aaron.ballman<br>
<br>
Subscribers: bogner, cfe-commits<br>
<br>
Differential Revision: <a href="https://reviews.llvm.org/D42887" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D42887</a><br>
<br>
Modified:<br>
cfe/trunk/docs/UsersManual.rst<br>
cfe/trunk/include/clang/<wbr>Driver/Options.td<br>
cfe/trunk/lib/Driver/<wbr>ToolChains/Clang.cpp<br>
cfe/trunk/test/Driver/clang_f_<wbr>opts.c<br>
<br>
Modified: cfe/trunk/docs/UsersManual.rst<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/docs/UsersManual.rst?rev=324498&r1=324497&r2=324498&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/cfe/trunk/docs/<wbr>UsersManual.rst?rev=324498&r1=<wbr>324497&r2=324498&view=diff</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- cfe/trunk/docs/UsersManual.rst (original)<br>
+++ cfe/trunk/docs/UsersManual.rst Wed Feb 7 10:36:51 2018<br>
@@ -1855,6 +1855,27 @@ features. You can "tune" the debug info<br>
must come first.)<br>
<br>
<br>
+Controlling LLVM IR Output<br>
+--------------------------<br>
+<br>
+Controlling Value Names in LLVM IR<br>
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^<wbr>^^^^^<br>
+<br>
+Emitting value names in LLVM IR increases the size and verbosity of the IR.<br>
+By default, value names are only emitted in assertion-enabled builds of Clang.<br>
+However, when reading IR it can be useful to re-enable the emission of value<br>
+names to improve readability.<br>
+<br>
+.. option:: -fdiscard-value-names<br>
+<br>
+ Discard value names when generating LLVM IR.<br>
+<br>
+.. option:: -fno-discard-value-names<br>
+<br>
+ Do not discard value names when generating LLVM IR. This option can be used<br>
+ to re-enable names for release builds of Clang.<br>
+<br>
+<br>
Comment Parsing Options<br>
-----------------------<br>
<br>
<br>
Modified: cfe/trunk/include/clang/<wbr>Driver/Options.td<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Driver/Options.td?rev=324498&r1=324497&r2=324498&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/cfe/trunk/include/<wbr>clang/Driver/Options.td?rev=<wbr>324498&r1=324497&r2=324498&<wbr>view=diff</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- cfe/trunk/include/clang/<wbr>Driver/Options.td (original)<br>
+++ cfe/trunk/include/clang/<wbr>Driver/Options.td Wed Feb 7 10:36:51 2018<br>
@@ -790,6 +790,10 @@ def fdiagnostics_show_template_<wbr>tree : Fl<br>
HelpText<"Print a template comparison tree for differing templates">;<br>
def fdeclspec : Flag<["-"], "fdeclspec">, Group<f_clang_Group>,<br>
HelpText<"Allow __declspec as a keyword">, Flags<[CC1Option]>;<br>
+def fdiscard_value_names : Flag<["-"], "fdiscard-value-names">, Group<f_clang_Group>,<br>
+ HelpText<"Discard value names in LLVM IR">, Flags<[DriverOption]>;<br>
+def fno_discard_value_names : Flag<["-"], "fno-discard-value-names">, Group<f_clang_Group>,<br>
+ HelpText<"Do not discard value names in LLVM IR">, Flags<[DriverOption]>;<br>
def fdollars_in_identifiers : Flag<["-"], "fdollars-in-identifiers">, Group<f_Group>,<br>
HelpText<"Allow '$' in identifiers">, Flags<[CC1Option]>;<br>
def fdwarf2_cfi_asm : Flag<["-"], "fdwarf2-cfi-asm">, Group<clang_ignored_f_Group>;<br>
<br>
Modified: cfe/trunk/lib/Driver/<wbr>ToolChains/Clang.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/ToolChains/Clang.cpp?rev=324498&r1=324497&r2=324498&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/cfe/trunk/lib/Driver/<wbr>ToolChains/Clang.cpp?rev=<wbr>324498&r1=324497&r2=324498&<wbr>view=diff</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- cfe/trunk/lib/Driver/<wbr>ToolChains/Clang.cpp (original)<br>
+++ cfe/trunk/lib/Driver/<wbr>ToolChains/Clang.cpp Wed Feb 7 10:36:51 2018<br>
@@ -3266,13 +3266,24 @@ void Clang::ConstructJob(<wbr>Compilation &C,<br>
if (!C.isForDiagnostics())<br>
CmdArgs.push_back("-disable-<wbr>free");<br>
<br>
-// Disable the verification pass in -asserts builds.<br>
#ifdef NDEBUG<br>
- CmdArgs.push_back("-disable-<wbr>llvm-verifier");<br>
- // Discard LLVM value names in -asserts builds.<br>
- CmdArgs.push_back("-discard-<wbr>value-names");<br>
+ const bool IsAssertBuild = false;<br>
+#else<br>
+ const bool IsAssertBuild = true;<br>
#endif<br>
<br>
+ // Disable the verification pass in -asserts builds.<br>
+ if (!IsAssertBuild)<br>
+ CmdArgs.push_back("disable-<wbr>llvm-verifier");<br>
+<br>
+ // Discard value names in assert builds unless otherwise specified.<br>
+ if (const Arg *A = Args.getLastArg(options::OPT_<wbr>fdiscard_value_names,<br>
+ options::OPT_fno_discard_<wbr>value_names)) {<br>
+ if (A->getOption().matches(<wbr>options::OPT_fdiscard_value_<wbr>names))<br></blockquote></div></div><div><br>Should this ^ be:<br><br> if (Args.hasFlag(options::OPT_<wbr>fdiscard_value_names, options::OPT_fno_discard_<wbr>value_names, false))<br></div></div></div></blockquote><div><br></div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>Yeah, that looks better, but perhaps this would be even cleaner:</div><div><br></div><div>if (Args.hasFlag(options::OPT_<wbr>fdiscard_value_names, options::OPT_fno_discard_<wbr>value_names, !IsAssertBuild))</div></div></div></div></blockquote></div></div><div><br>*nod* Looks pretty good to me.<br> </div><div><div class="m_3385180945354387933m_-6294943521944150138h5"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div><br>instead? (simpler, one operation instead of two, etc)<br> </div><span><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ CmdArgs.push_back("-discard-<wbr>value-names");<br>
+ } else if (!IsAssertBuild)<br>
+ CmdArgs.push_back("-discard-<wbr>value-names");<br>
+<br>
// Set the main file name, so that debug info works even with<br>
// -save-temps.<br>
CmdArgs.push_back("-main-<wbr>file-name");<br>
<br>
Modified: cfe/trunk/test/Driver/clang_f_<wbr>opts.c<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Driver/clang_f_opts.c?rev=324498&r1=324497&r2=324498&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/cfe/trunk/test/Driver/<wbr>clang_f_opts.c?rev=324498&r1=<wbr>324497&r2=324498&view=diff</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- cfe/trunk/test/Driver/clang_f_<wbr>opts.c (original)<br>
+++ cfe/trunk/test/Driver/clang_f_<wbr>opts.c Wed Feb 7 10:36:51 2018<br>
@@ -517,3 +517,8 @@<br>
// RUN: %clang -### -S %s 2>&1 | FileCheck -check-prefix=CHECK-NO-CF-<wbr>PROTECTION-BRANCH %s<br>
// CHECK-CF-PROTECTION-BRANCH: -fcf-protection=branch<br>
// CHECK-NO-CF-PROTECTION-BRANCH-<wbr>NOT: -fcf-protection=branch<br>
+<br>
+// RUN: %clang -### -S -fdiscard-value-names %s 2>&1 | FileCheck -check-prefix=CHECK-DISCARD-<wbr>NAMES %s<br></blockquote></span><div><br>Should there also be a RUN line here for the default behavior, when no arg is specified?<br></div></div></div></blockquote><div><br></div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>AFAIK it is not currently testable, since the result of the test depends on the build configuration, and AFAIK that information</div><div>isn't available in the LIT test suite.</div><div><br></div><div>I'm sure with a bit of messy CMake I could find a way to transmit if assertions are enabled to LIT, add it to the set of available</div><div>features, and write two different tests that have REQUIRE/UNSUPPORTED lines which turn them on/off depending on</div><div>the build type.</div><div><br></div><div>However, this behavior has gone untested for years, so I wasn't sure a test was required now.</div></div></div></div></blockquote></div></div><div><br>Generally good to try to raise the bar a bit where it's been dropped in parts of the codebase.<br><br>But, yes, since the default changes - you should be able to test the default in an asserts build correctly (since there's a REQUIRES: Asserts) but we've deliberately avoided putting in a REQUIRES: NoAsserts, because generally there should be no functionality behind an assertion failure - but this sort of behavior slips through the cracks. Meh.<br></div></div></div></blockquote><div><br></div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>Ah, OK. I didn't see that when I went looking. I'll add tests then. Thankfully I don't need `NoAsserts` since I can write `// UNSUPPORTED: Asserts` instead :-)</div></div></div></div></blockquote></div></div></div></blockquote><div><br></div><div>After further investigation, the `asserts` feature isn't suitable for use here, in Clang. The value is determined by `llvm-config --assertion-mode`, which may be different</div><div>from the assertion mode of the Clang in standalone builds.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div> </div><span><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div> </div><span><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+// RUN: %clang -### -S -fno-discard-value-names %s 2>&1 | FileCheck -check-prefix=CHECK-NO-<wbr>DISCARD-NAMES %s<br>
+// CHECK-DISCARD-NAMES: "-discard-value-names"<br>
+// CHECK-NO-DISCARD-NAMES-NOT: "-discard-value-names"<br>
<br>
<br>
______________________________<wbr>_________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/<wbr>mailman/listinfo/cfe-commits</a><br>
</blockquote></span></div></div>
</blockquote></div></div></div></blockquote></span></div></div>
</blockquote></div></div></div></blockquote></div>
</div></div></blockquote></div><br></div></div>