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

Mon Feb 12 18:07:24 PST 2018

Ah.. hrm :/

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

> 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/20180213/52f7cbd7/attachment-0001.html>