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

Mon Feb 12 15:01:39 PST 2018

ah, sweet :)

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

> On Mon, Feb 12, 2018 at 3:35 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>>
>>
>> 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.
>>
>
> 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 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/3f4750f8/attachment-0001.html>