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. > > >> >> >>> +// 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
