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
