Optimize C++ Backend for Conditional Fields #234
Optimize C++ Backend for Conditional Fields #234AaronWebster wants to merge 13 commits intogoogle:masterfrom
Conversation
Optimized the generated C++ code for structures with conditional fields
by identifying and caching repeated subexpressions (e.g., tag reads)
in the `Ok()`, `Equals()`, and `UncheckedEquals()` methods.
This approach uses a two-pass generation strategy:
1. Count usage of existence condition subexpressions.
2. Generate code where frequently used subexpressions are cached in
local variables at the method scope, while unique expressions are
inlined.
This reduces redundant computations (CPU optimization) without
introducing excessive stack usage for unique conditions (Memory
optimization).
Also updated test infrastructure to support passing compiler flags to
golden tests, fixing `no_enum_traits_golden_test`.
Added `testdata/many_conditionals.emb` and `compiler/back_end/cpp/testcode/many_conditionals_benchmark.cc`
to verify performance of conditional fields.
Performance Analysis (10,000 iterations x 100 tags on `testdata/many_conditionals.emb`):
- Baseline Runtime: ~1.83s
- Optimized Runtime: ~0.21s (Speedup: ~8.7x)
Binary Size Impact (many_conditionals_benchmark, -c opt):
- Baseline: 57848 bytes (text: 56112)
- Optimized: 30472 bytes (text: 28736) -> ~47% reduction
…aronWebster/emboss into copilot/fix-all-build-files-and-tests
Co-authored-by: AaronWebster <3766083+AaronWebster@users.noreply.github.com>
…nd-tests Fix C++ backend conditional optimization: formatting and golden file regeneration
EricRahm
left a comment
EricRahm
left a comment
There was a problem hiding this comment.
Sorry for the post-commit review, I just want to double-check your intent. I have some minor concerns about raw code size increase and possible compile time regressions. I can run this against pigweed's emboss defs and follow up with numbers.
| // Ok(). | ||
| if (has_${field}.ValueOrDefault() && !${field}.Ok()) return false; | ||
| { | ||
| const auto emboss_reserved_local_field_present = ${existence_condition}; |
There was a problem hiding this comment.
So I think this is intended to inline the contents of has_<field>, is that correct? And the goal of that is to reuse a cached instance of a field that is referenced in multiple has_<field> methods? Looking at the generated code now it's...pretty verbose. I'm actually a little surprised this would reduce code size.
There was a problem hiding this comment.
Check out #235 which I think is more correct as to what Ben suggested I implement and the code is much cleaner. Some compiler specific attributes could be elided, let me know.
|
|
||
| if (!has_x().Known()) return false; | ||
| if (has_x().ValueOrDefault() && !x().Ok()) return false; | ||
| const auto emboss_reserved_local_ok_subexpr_1 = x(); |
There was a problem hiding this comment.
I guess this is an example of what we're trying to optimize.
| { | ||
| const auto emboss_reserved_local_field_present = ::emboss::support::Maybe</**/bool>(true); | ||
| if (!emboss_reserved_local_field_present.Known()) return false; | ||
| if (emboss_reserved_local_field_present.ValueOrDefault() && !x().Ok()) return false; |
There was a problem hiding this comment.
Although note we don't use it here.
| if (has_xc().ValueOrDefault() && !xc().Ok()) return false; | ||
|
|
||
| { | ||
| const auto emboss_reserved_local_field_present = ::emboss::support::Equal</**/::std::int32_t, bool, ::std::int32_t, ::std::int32_t>((emboss_reserved_local_ok_subexpr_1.Ok() ? ::emboss::support::Maybe</**/::std::int32_t>(static_cast</**/::std::int32_t>(emboss_reserved_local_ok_subexpr_1.UncheckedRead())) : ::emboss::support::Maybe</**/::std::int32_t>()), ::emboss::support::Maybe</**/::std::int32_t>(static_cast</**/::std::int32_t>(0LL))); |
| if (has_xcc().ValueOrDefault() && !xcc().Ok()) return false; | ||
|
|
||
| { | ||
| const auto emboss_reserved_local_field_present = ::emboss::support::And</**/bool, bool, bool, bool>(::emboss::support::Equal</**/::std::int32_t, bool, ::std::int32_t, ::std::int32_t>((emboss_reserved_local_ok_subexpr_1.Ok() ? ::emboss::support::Maybe</**/::std::int32_t>(static_cast</**/::std::int32_t>(emboss_reserved_local_ok_subexpr_1.UncheckedRead())) : ::emboss::support::Maybe</**/::std::int32_t>()), ::emboss::support::Maybe</**/::std::int32_t>(static_cast</**/::std::int32_t>(0LL))), ::emboss::support::Equal</**/::std::int32_t, bool, ::std::int32_t, ::std::int32_t>((xc().Ok() ? ::emboss::support::Maybe</**/::std::int32_t>(static_cast</**/::std::int32_t>(xc().UncheckedRead())) : ::emboss::support::Maybe</**/::std::int32_t>()), ::emboss::support::Maybe</**/::std::int32_t>(static_cast</**/::std::int32_t>(0LL)))); |
There was a problem hiding this comment.
...and here. Note how verbose this has become though. Maybe that's fine, this is generated code after all.
|
Eric, can you please move your comments to #235 so we can have the discussion there? |
This change significantly optimizes the generated C++ code for
structures utilizing conditional fields.
Key optimizations include:
frequently used subexpressions (e.g., tag reads) within Ok(),
Equals(), and UncheckedEquals() methods.
by a second pass that caches repeated expressions in local
variables while inlining unique ones.
performance improvements and reduced stack usage for unique
conditions.
Performance Impact (on testdata/many_conditionals.emb with 10,000
iterations x 100 tags):
bytes for many_conditionals_benchmark with -c opt).
Additional updates:
golden tests, resolving an issue with no_enum_traits_golden_test.
(compiler/back_end/cpp/testcode/many_conditionals_benchmark.cc)
were added to verify performance.