This is an archive of the discontinued LLVM Phabricator instance.

[OptTable] Precompute OptTable prefixes union table through tablegen
ClosedPublic

Authored by serge-sans-paille on Dec 31 2022, 4:15 AM.

Details

Summary

This avoid rediscovering this table when reading each options, providing
a sensible 2% speedup when processing and empty file, and a measurable
speedup on typical workloads, see:

https://llvm-compile-time-tracker.com/compare.php?from=4da6cb3202817ee2897d6b690e4af950459caea4&to=dffbe8638a92bfc73dff5494144d5ef9a73c0acc&stat=instructions:u

Diff Detail

Event Timeline

Herald added a reviewer: MaskRay. · View Herald Transcript
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
serge-sans-paille requested review of this revision.Dec 31 2022, 4:15 AM
Herald added projects: Restricted Project, Restricted Project, Restricted Project. · View Herald TranscriptDec 31 2022, 4:15 AM
thakis added a subscriber: thakis.Jan 1 2023, 8:29 AM

Could we make this an optional opt-in maybe? I imagine for the majority of the tools, this has no measurable effect, but it currently makes the Opt library harder to use for every client.

Could we make this an optional opt-in maybe? I imagine for the majority of the tools, this has no measurable effect, but it currently makes the Opt library harder to use for every client.

Sure, let's have two constructors, one that uses a precomputed table and one that does the precomputation.
Would you prefer having only clang use the faster-but-less efficient method, or can I keep the changes as is?

I'd only use it in clang tbh. Clang uses many orders of magnitudes more opts than all other tools, and even there it's not a _huge_ win. (2% on empty file is very nice! But most of the time, people don't compile empty files, and I'm guessing on non-empty files this is a negligible win even in clang.) What do you think?

Added to versions of OptTable, one that uses a precomputed Table, generated by TableGen, and one that relies on the old, generic approach. Both are tested using the same test bench.

Updated benchmark: https://llvm-compile-time-tracker.com/compare.php?from=4da6cb3202817ee2897d6b690e4af950459caea4&to=19a492b704e8f5c1dea120b9c0d3859bd78796be&stat=instructions:u

I'm guessing on non-empty files this is a negligible win even in clang.

As showcased by https://llvm-compile-time-tracker.com/compare.php?from=4da6cb3202817ee2897d6b690e4af950459caea4&to=19a492b704e8f5c1dea120b9c0d3859bd78796be&stat=instructions:u, it's not neglectible in -O0 (and obviously even less neglictible for the preprocessing-only step)

(rebased on main. Note that this patch is applied on top of https://reviews.llvm.org/D140699)

serge-sans-paille retitled this revision from Precompute OptTable prefixes union table through tablegen to [OptTable] Precompute OptTable prefixes union table through tablegen.Jan 3 2023, 12:50 AM
nikic added a comment.Jan 11 2023, 7:36 AM

Just to check, this isn't going to cause some warning spew about all those OptTable implementations being non-final?

llvm/include/llvm/Option/OptTable.h
256

Comment looks a bit unfinished :)

llvm/unittests/Option/OptionParsingTest.cpp
87

What's this about?

Nits + rebase on main branch

serge-sans-paille marked 2 inline comments as done.Jan 11 2023, 12:48 PM

Just to check, this isn't going to cause some warning spew about all those OptTable implementations being non-final?

nope. Why would there be?

llvm/unittests/Option/OptionParsingTest.cpp
327
nikic accepted this revision.Jan 12 2023, 2:09 AM

LGTM

Just to check, this isn't going to cause some warning spew about all those OptTable implementations being non-final?

nope. Why would there be?

Something about missing virtual destructors on a non-final class? Not sure when exactly that warning applies.

llvm/unittests/Option/OptionParsingTest.cpp
327

Huh, TIL.

This revision is now accepted and ready to land.Jan 12 2023, 2:09 AM
This revision was landed with ongoing or failed builds.Jan 12 2023, 3:08 AM
This revision was automatically updated to reflect the committed changes.

Thanks for this change :)

clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp
141

C

141

…not sure what this was, please ignore :)

llvm/include/llvm/Option/OptTable.h
257

(nit, kind of too late now: I would've renamed the base class to BaseOptTable or what and kept OptTable for this one, so that all the tools wouldn't have to be touched at all.)