Eliminate redundant residual re-read in optimized Ok() switch arms by AaronWebster · Pull Request #272 · google/emboss

AaronWebster · 2026-07-19T19:18:10Z

Based on #271 (the guard-demote correctness precursor); review/merge that first.

What

In the optimized Ok() switch, each residual-guarded arm previously re-read and
re-compared the discriminant that the switch had already decoded, then recomputed
has_X(). This gates each residual arm on the residual alone:

if (!(<R>).Known()) return false;
if ((<R>).ValueOrDefault() && !X().Ok()) return false;

dropping the redundant discriminant re-read. For more than one conjunct the residual is
rendered once as a synthesized boolean-AND expression (shared subexpressions). The gate
is required, not merely an optimization: X() internally re-checks has_X() and returns
a null view when the field is absent, so an unguarded X().Ok() would wrongly fail.

Soundness rests on the #271 invariant: every switch reaching _emit_switch_block has the
discriminant Known inside case K: (provably, or via the emitted Known() guard), so
has_X() ⟺ residual.

Measurement

Adds a residual-heavy OuterGuardedUnion schema to testdata/many_conditionals.emb
(always-present outer, ~100 arms guarded on tag == K && outer == 1) so Lever A
actually fires and the committed harness (#270) can measure it. The flat
LargeConditionals union is all bare arms and is the unaffected control.

OuterGuardedUnion::Ok() reader symbol @ -Os, vs the #271 baseline:

Target	before → after	Δ
MicroBlaze LE (Vitis, authoritative)	16748 → 13620	−18.7%
MicroBlaze BE	13960 → 10964	−21.5%
ARM Cortex-M4	9296 → 7716	−17.0%
x86-64 (g++)	—	−9.5%
x86-64 (clang)	—	−50.7%

Retired instructions (OuterGuardedUnion hot loop): MB-BE −3.2%, MB-LE −1.6%, ARM −1.1%
(small — only the one active arm runs per call).

Control LargeConditionals: byte-identical codegen, 0 symbol delta on every target.
(The plan predicted x86 size unchanged; Lever A in fact helps x86 too.)

Verification

Golden diff confined to condition.emb.h + many_conditionals.emb.h; all pre-existing
structs byte-identical.
Full bazel test //... green (206/206), including condition_test's Guard conditional switch discriminants in optimized Ok() #266 oracle.

Lever A of the Ok() rework: in a conditional-switch arm whose field carries a residual predicate beyond the discriminant equality (e.g. `if tag == K && outer == 1`), gate the field's Ok() check on the residual alone rather than re-deriving the full has_<field>() existence condition. Inside `case K:` the switch has already decoded and (provably, or via the emitted Known() guard) established the discriminant, so `has_<field>() <=> residual`. The prior form re-invoked has_<field>(), which redundantly re-reads and re-compares the discriminant on every arm; the residual (a single hoisted, shared subexpression) drops that per-arm work. Implementation: preserve the residual conjunct list through _generate_optimized_ok_method_body instead of collapsing it to a bool, thread (ir, subexpressions) into _emit_switch_block/_render_case_body, and render the residual at emit time (so demoted groups leak no dead subexpression). Multiple conjuncts fold into one synthesized boolean AND so the residual renders once. The gate is required for correctness, not just size: <field>() returns a null view when absent, so an unguarded <field>().Ok() would wrongly fail when the residual is false; we also bail when the residual is not Known. Adds testdata OuterGuardedUnion (100 tag arms each guarded by a shared `outer == 1` residual) to exercise and measure the change; flat LargeConditionals (all bare arms) is the unaffected control. Measured OuterGuardedUnion::Ok() reader symbol @ -Os vs the pre-Lever-A generator (schema held fixed): MicroBlaze LE/Vitis (authoritative) -18.7%, MicroBlaze BE -21.5%, ARM Cortex-M4 -17.0%, x86-64 gcc -9.5% / clang -50.7%. Retired guest instructions (OuterGuardedUnion hot loop): MicroBlaze BE -3.2%, MicroBlaze LE -1.6%, ARM -1.1%. Flat LargeConditionals codegen is byte-identical (0 symbol delta on every target); x86 wall-clock unchanged. Golden diff is confined to the residual-arm schemas (condition.emb.h, many_conditionals.emb.h); every pre-existing struct is byte-identical. Full compiler+runtime suite (205 tests) and the #266 condition_test oracle stay green.

robrussell · 2026-07-22T23:10:13Z

-                    case_entry["entries"].append((field, bool(residual)))
+                    # Keep the residual conjunct list itself (not just a bool):
+                    # a residual arm gates its Ok() check on the residual alone
+                    # at emit time (Lever A), so the concrete IR must survive.


This comment seems unnecessary.

robrussell · 2026-07-22T23:11:40Z

+            not residual
            for case_entry in group["cases_by_label"].values()
-            for (_field, has_residual) in case_entry["entries"]
+            for (_field, residual) in case_entry["entries"]


for (_, residual) in case_entry["entries"]

robrussell · 2026-07-22T23:13:36Z

+    this case. When the list is empty the arm is bare and the case body is
+    a single direct Ok() check.
+
+    When there is a residual, we gate the Ok() check on the residual *alone*


"When the list of residuals is not empty..."

robrussell · 2026-07-22T23:14:05Z

-    then trusted to fold the now-trivially-true discriminant comparison
-    inside the has_${field}() call (it's inlined and the case label has
-    pinned the discriminant value).
+    Each entry is `(field, residual)` where `residual` is the list of


residuals

robrussell · 2026-07-22T23:14:19Z

    """
    parts = []
-    for field, has_residual in entries:
+    for field, residual in entries:


residuals

robrussell · 2026-07-22T23:30:59Z

+    `has_${field}()` internally and returns a null view (whose Ok() is false)
+    when the field is absent, so an unguarded `${field}().Ok()` would wrongly
+    fail whenever the residual is false. We first bail if the residual is not
+    Known (e.g. an out-of-bounds read), matching the has_${field}()-based


"matching the has_${field}()-based check it replaces." refers to code that no longer exists after this PR.

The explanation is somewhat useful but overly verbose and includes a lot of implementation details about this function and others which may not hold true in future revisions.

robrussell · 2026-07-22T23:48:05Z

+    for field, residual in entries:
        name = _cpp_field_name(field.name.name.text)
-        if has_residual:
+        if residual:


These branches can be simplified, something like:

for field, residuals in entries: name = _cpp_field_name(field.name.name.text) if len(residuals) == 0: parts.append(" if (!{}().Ok()) return false;\n".format(name)) continue if len(residuals) == 1: residual = residuals[0] else: residual = ir_data.Expression( function=ir_data.Function( function=ir_data.FunctionMapping.AND, args=residuals, ), type=ir_data.ExpressionType(boolean=ir_data.BooleanType()), ) rendered = _render_expression( residual, ir, subexpressions=subexpressions ).rendered parts.append( " if (!({0}).Known()) return false;\n if (({1}).ValueOrDefault() && !{2}().Ok()) return false;\n" .format(rendered, rendered, name ) )

AaronWebster requested a review from robrussell July 19, 2026 19:20

This was referenced Jul 19, 2026

Statically-known existence conditions: drop or simplify the check #259

Closed

Tail-form return in terminal switches #258

Closed

robrussell approved these changes Jul 22, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Eliminate redundant residual re-read in optimized Ok() switch arms#272

Eliminate redundant residual re-read in optimized Ok() switch arms#272
AaronWebster wants to merge 1 commit into
emboss/ok-guard-demotefrom
emboss/ok-residual-elim

AaronWebster commented Jul 19, 2026

Uh oh!

robrussell Jul 22, 2026

Uh oh!

robrussell Jul 22, 2026

Uh oh!

robrussell Jul 22, 2026

Uh oh!

robrussell Jul 22, 2026

Uh oh!

robrussell Jul 22, 2026

Uh oh!

robrussell Jul 22, 2026

Uh oh!

robrussell Jul 22, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

AaronWebster commented Jul 19, 2026

What

Measurement

Verification

Uh oh!

robrussell Jul 22, 2026

Choose a reason for hiding this comment

Uh oh!

robrussell Jul 22, 2026

Choose a reason for hiding this comment

Uh oh!

robrussell Jul 22, 2026

Choose a reason for hiding this comment

Uh oh!

robrussell Jul 22, 2026

Choose a reason for hiding this comment

Uh oh!

robrussell Jul 22, 2026

Choose a reason for hiding this comment

Uh oh!

robrussell Jul 22, 2026

Choose a reason for hiding this comment

Uh oh!

robrussell Jul 22, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants