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

Eric Fiselier via cfe-commits cfe-commits at lists.llvm.org
Mon Feb 12 18:03:42 PST 2018 

On Mon, Feb 12, 2018 at 4:01 PM, David Blaikie <dblaikie at gmail.com> wrote:

> 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 :-)
>>
>
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 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/98ef091c/attachment-0001.html>

More information about the cfe-commits mailing list