r324498 - [Driver] Add option to manually control discarding value names in LLVM IR.

Mon Feb 12 14:35:31 PST 2018

On Mon, Feb 12, 2018 at 2:25 PM Eric Fiselier <eric at efcs.ca> wrote:

> On Mon, Feb 12, 2018 at 9:15 AM, David Blaikie <dblaikie at gmail.com> wrote:
>
>>
>>
>> On Wed, Feb 7, 2018 at 10:38 AM Eric Fiselier via cfe-commits <
>> cfe-commits at 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 at lists.llvm.org
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20180212/b5e03231/attachment.html>