ah, sweet :) On Mon, Feb 12, 2018 at 2:59 PM Eric Fiselier <e...@efcs.ca> wrote:
> On Mon, Feb 12, 2018 at 3:35 PM, David Blaikie <dblai...@gmail.com> wrote: > >> >> >> On Mon, Feb 12, 2018 at 2:25 PM Eric Fiselier <e...@efcs.ca> wrote: >> >>> On Mon, Feb 12, 2018 at 9:15 AM, David Blaikie <dblai...@gmail.com> >>> wrote: >>> >>>> >>>> >>>> On Wed, Feb 7, 2018 at 10:38 AM Eric Fiselier via cfe-commits < >>>> cfe-commits@lists.llvm.org> wrote: >>>> >>>>> Author: ericwf >>>>> Date: Wed Feb 7 10:36:51 2018 >>>>> New Revision: 324498 >>>>> >>>>> URL: http://llvm.org/viewvc/llvm-project?rev=324498&view=rev >>>>> Log: >>>>> [Driver] Add option to manually control discarding value names in LLVM >>>>> IR. >>>>> >>>>> Summary: >>>>> 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. >>>>> >>>>> 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. >>>>> >>>> >>>> It wouldn't necessarily have to invoke CC1, it could use "-Xclang >>>> -discard-value-names". >>>> >>> >>> 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: >>> 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 >>> "-discard-value-names". >>> >> >> Ah, thanks for explaining! >> >> >>> >>> >>>> >>>> >>>>> >>>>> 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. >>>>> >>>>> Reviewers: erichkeane, aaron.ballman, lebedev.ri >>>>> >>>>> Reviewed By: aaron.ballman >>>>> >>>>> Subscribers: bogner, cfe-commits >>>>> >>>>> Differential Revision: https://reviews.llvm.org/D42887 >>>>> >>>>> Modified: >>>>> cfe/trunk/docs/UsersManual.rst >>>>> cfe/trunk/include/clang/Driver/Options.td >>>>> cfe/trunk/lib/Driver/ToolChains/Clang.cpp >>>>> cfe/trunk/test/Driver/clang_f_opts.c >>>>> >>>>> Modified: cfe/trunk/docs/UsersManual.rst >>>>> URL: >>>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/docs/UsersManual.rst?rev=324498&r1=324497&r2=324498&view=diff >>>>> >>>>> ============================================================================== >>>>> --- cfe/trunk/docs/UsersManual.rst (original) >>>>> +++ cfe/trunk/docs/UsersManual.rst Wed Feb 7 10:36:51 2018 >>>>> @@ -1855,6 +1855,27 @@ features. You can "tune" the debug info >>>>> must come first.) >>>>> >>>>> >>>>> +Controlling LLVM IR Output >>>>> +-------------------------- >>>>> + >>>>> +Controlling Value Names in LLVM IR >>>>> +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ >>>>> + >>>>> +Emitting value names in LLVM IR increases the size and verbosity of >>>>> the IR. >>>>> +By default, value names are only emitted in assertion-enabled builds >>>>> of Clang. >>>>> +However, when reading IR it can be useful to re-enable the emission >>>>> of value >>>>> +names to improve readability. >>>>> + >>>>> +.. option:: -fdiscard-value-names >>>>> + >>>>> + Discard value names when generating LLVM IR. >>>>> + >>>>> +.. option:: -fno-discard-value-names >>>>> + >>>>> + Do not discard value names when generating LLVM IR. This option can >>>>> be used >>>>> + to re-enable names for release builds of Clang. >>>>> + >>>>> + >>>>> Comment Parsing Options >>>>> ----------------------- >>>>> >>>>> >>>>> Modified: cfe/trunk/include/clang/Driver/Options.td >>>>> URL: >>>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Driver/Options.td?rev=324498&r1=324497&r2=324498&view=diff >>>>> >>>>> ============================================================================== >>>>> --- cfe/trunk/include/clang/Driver/Options.td (original) >>>>> +++ cfe/trunk/include/clang/Driver/Options.td Wed Feb 7 10:36:51 2018 >>>>> @@ -790,6 +790,10 @@ def fdiagnostics_show_template_tree : Fl >>>>> HelpText<"Print a template comparison tree for differing >>>>> templates">; >>>>> def fdeclspec : Flag<["-"], "fdeclspec">, Group<f_clang_Group>, >>>>> HelpText<"Allow __declspec as a keyword">, Flags<[CC1Option]>; >>>>> +def fdiscard_value_names : Flag<["-"], "fdiscard-value-names">, >>>>> Group<f_clang_Group>, >>>>> + HelpText<"Discard value names in LLVM IR">, Flags<[DriverOption]>; >>>>> +def fno_discard_value_names : Flag<["-"], "fno-discard-value-names">, >>>>> Group<f_clang_Group>, >>>>> + HelpText<"Do not discard value names in LLVM IR">, >>>>> Flags<[DriverOption]>; >>>>> def fdollars_in_identifiers : Flag<["-"], "fdollars-in-identifiers">, >>>>> Group<f_Group>, >>>>> HelpText<"Allow '$' in identifiers">, Flags<[CC1Option]>; >>>>> def fdwarf2_cfi_asm : Flag<["-"], "fdwarf2-cfi-asm">, >>>>> Group<clang_ignored_f_Group>; >>>>> >>>>> Modified: cfe/trunk/lib/Driver/ToolChains/Clang.cpp >>>>> URL: >>>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/ToolChains/Clang.cpp?rev=324498&r1=324497&r2=324498&view=diff >>>>> >>>>> ============================================================================== >>>>> --- cfe/trunk/lib/Driver/ToolChains/Clang.cpp (original) >>>>> +++ cfe/trunk/lib/Driver/ToolChains/Clang.cpp Wed Feb 7 10:36:51 2018 >>>>> @@ -3266,13 +3266,24 @@ void Clang::ConstructJob(Compilation &C, >>>>> if (!C.isForDiagnostics()) >>>>> CmdArgs.push_back("-disable-free"); >>>>> >>>>> -// Disable the verification pass in -asserts builds. >>>>> #ifdef NDEBUG >>>>> - CmdArgs.push_back("-disable-llvm-verifier"); >>>>> - // Discard LLVM value names in -asserts builds. >>>>> - CmdArgs.push_back("-discard-value-names"); >>>>> + const bool IsAssertBuild = false; >>>>> +#else >>>>> + const bool IsAssertBuild = true; >>>>> #endif >>>>> >>>>> + // Disable the verification pass in -asserts builds. >>>>> + if (!IsAssertBuild) >>>>> + CmdArgs.push_back("disable-llvm-verifier"); >>>>> + >>>>> + // Discard value names in assert builds unless otherwise specified. >>>>> + if (const Arg *A = >>>>> Args.getLastArg(options::OPT_fdiscard_value_names, >>>>> + >>>>> options::OPT_fno_discard_value_names)) { >>>>> + if (A->getOption().matches(options::OPT_fdiscard_value_names)) >>>>> >>>> >>>> Should this ^ be: >>>> >>>> if (Args.hasFlag(options::OPT_fdiscard_value_names, >>>> options::OPT_fno_discard_value_names, false)) >>>> >>> >>> Yeah, that looks better, but perhaps this would be even cleaner: >>> >>> if (Args.hasFlag(options::OPT_fdiscard_value_names, >>> options::OPT_fno_discard_value_names, !IsAssertBuild)) >>> >> >> *nod* Looks pretty good to me. >> >> >>> >>> >>>> >>>> instead? (simpler, one operation instead of two, etc) >>>> >>>> >>>>> + CmdArgs.push_back("-discard-value-names"); >>>>> + } else if (!IsAssertBuild) >>>>> + CmdArgs.push_back("-discard-value-names"); >>>>> + >>>>> // Set the main file name, so that debug info works even with >>>>> // -save-temps. >>>>> CmdArgs.push_back("-main-file-name"); >>>>> >>>>> Modified: cfe/trunk/test/Driver/clang_f_opts.c >>>>> URL: >>>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Driver/clang_f_opts.c?rev=324498&r1=324497&r2=324498&view=diff >>>>> >>>>> ============================================================================== >>>>> --- cfe/trunk/test/Driver/clang_f_opts.c (original) >>>>> +++ cfe/trunk/test/Driver/clang_f_opts.c Wed Feb 7 10:36:51 2018 >>>>> @@ -517,3 +517,8 @@ >>>>> // RUN: %clang -### -S %s 2>&1 | FileCheck >>>>> -check-prefix=CHECK-NO-CF-PROTECTION-BRANCH %s >>>>> // CHECK-CF-PROTECTION-BRANCH: -fcf-protection=branch >>>>> // CHECK-NO-CF-PROTECTION-BRANCH-NOT: -fcf-protection=branch >>>>> + >>>>> +// RUN: %clang -### -S -fdiscard-value-names %s 2>&1 | FileCheck >>>>> -check-prefix=CHECK-DISCARD-NAMES %s >>>>> >>>> >>>> Should there also be a RUN line here for the default behavior, when no >>>> arg is specified? >>>> >>> >>> AFAIK it is not currently testable, since the result of the test depends >>> on the build configuration, and AFAIK that information >>> isn't available in the LIT test suite. >>> >>> 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 >>> features, and write two different tests that have REQUIRE/UNSUPPORTED >>> lines which turn them on/off depending on >>> the build type. >>> >>> However, this behavior has gone untested for years, so I wasn't sure a >>> test was required now. >>> >> >> Generally good to try to raise the bar a bit where it's been dropped in >> parts of the codebase. >> >> 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. >> > > 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 :-) > >> >> >>> >>> >>>> >>>> >>>>> +// RUN: %clang -### -S -fno-discard-value-names %s 2>&1 | FileCheck >>>>> -check-prefix=CHECK-NO-DISCARD-NAMES %s >>>>> +// CHECK-DISCARD-NAMES: "-discard-value-names" >>>>> +// CHECK-NO-DISCARD-NAMES-NOT: "-discard-value-names" >>>>> >>>>> >>>>> _______________________________________________ >>>>> cfe-commits mailing list >>>>> cfe-commits@lists.llvm.org >>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits >>>>> >>>>
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
