Ah.. hrm :/ On Mon, Feb 12, 2018 at 6:03 PM Eric Fiselier <e...@efcs.ca> wrote:
> On Mon, Feb 12, 2018 at 4:01 PM, David Blaikie <dblai...@gmail.com> wrote: > >> 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 :-) >>> >> > 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 > from the assertion mode of the Clang in standalone builds. > > >> >>>> >>>>> >>>>> >>>>>> >>>>>> >>>>>>> +// 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
