Page MenuHomePhabricator

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

Authored by EricWF on Feb 3 2018, 2:25 PM.

Details

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.

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.

Diff Detail

Repository
rC Clang

Event Timeline

EricWF created this revision.Feb 3 2018, 2:25 PM
lebedev.ri added inline comments.Feb 3 2018, 2:44 PM
lib/Driver/ToolChains/Clang.cpp
3269

This logic seems sidewards.
If NDEBUG is specified, then it is assert()-less build.
If NDEBUG is *not* specified, then assert()'s are actually functional.

3270

-assert is assert()-less build

EricWF marked an inline comment as done.Feb 3 2018, 2:46 PM
EricWF added inline comments.
lib/Driver/ToolChains/Clang.cpp
3269

Woops! Silly me. Thats a dumb mistake and I feel dumb having made it.

EricWF updated this revision to Diff 132752.Feb 3 2018, 2:47 PM
EricWF marked an inline comment as done.
  • Address really really really dumb mistake.
lebedev.ri added inline comments.Feb 4 2018, 2:31 AM
test/Driver/clang_f_opts.c
522

I wonder if it is also possible to check that if neither of -f[no-]discard-value-names is specified, what happens.
The caveat is of course the asserts-vs-no-asserts build type.

aaron.ballman added inline comments.Feb 4 2018, 7:32 AM
docs/UsersManual.rst
1859

Underlining is incorrect here (too long).

1861

s/Values/Value

1862

Same here.

1865

By default value -> By default, value
in assertion enabled builds -> in assertion-enabled builds

bogner added a subscriber: bogner.Feb 5 2018, 1:12 PM
bogner added inline comments.
lib/Driver/ToolChains/Clang.cpp
3269–3274

It might be a few more characters, but I feel like this is more readable if you put entire statements in the branches of the #if, ie:

#ifdef NDEBUG

const bool IsAssertBuild = false;

#else

const bool IsAssertBuild = true;

#endif

EricWF marked 5 inline comments as done.Feb 7 2018, 10:30 AM
EricWF added inline comments.
lib/Driver/ToolChains/Clang.cpp
3269–3274

Ack. Done.

test/Driver/clang_f_opts.c
522

I don't think so, at least not easily and without changes to the lit configuration.

It's gone untested this long, I would love to get this patch in without being responsible for adding those tests.

EricWF updated this revision to Diff 133251.Feb 7 2018, 10:31 AM
EricWF marked an inline comment as done.
  • Fix documentation as requested.
  • Put entire statement inside of #ifdef blocks.
This revision is now accepted and ready to land.Feb 7 2018, 10:35 AM
This revision was automatically updated to reflect the committed changes.