Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
AaronWebster wants to merge 1 commit into
base: emboss/ok-guard-demote
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 50 additions & 21 deletions compiler/back_end/cpp/header_generator.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1663,7 +1663,10 @@ def _generate_optimized_ok_method_body(fields, ir, subexpressions):
# case label twice (`tag == 0 || tag == 0`); after #9 the
# simplifier should normally prevent this from reaching us.
if not any(e[0] is field for e in case_entry["entries"]):
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.

@robrussell robrussell Jul 22, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment seems unnecessary.

case_entry["entries"].append((field, residual))
field_group_key[id(field)] = key
else:
cond_res = _render_expression(
Expand Down Expand Up @@ -1718,9 +1721,9 @@ def _generate_optimized_ok_method_body(fields, ir, subexpressions):
group["type"] = "demoted_to_if"
continue
has_bare_arm = any(
not has_residual
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"]

@robrussell robrussell Jul 22, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

)
if not has_bare_arm and not _is_discriminant_provably_known(
group["discrim_expr"], fields
Expand All @@ -1740,7 +1743,7 @@ def _generate_optimized_ok_method_body(fields, ir, subexpressions):
group["known_check_required"] = not _is_discriminant_provably_known(
group["discrim_expr"], fields
)
blocks.append(_emit_switch_block(group))
blocks.append(_emit_switch_block(group, ir, subexpressions))
elif group["type"] == "demoted_to_if":
for field in group["encounter_order"]:
blocks.append(
Expand All @@ -1761,37 +1764,63 @@ def _generate_optimized_ok_method_body(fields, ir, subexpressions):
return "".join(blocks)


def _render_case_body(entries):
def _render_case_body(entries, ir, subexpressions):
"""Renders the body of a single switch arm.

Each entry is `(field, has_residual)` where `has_residual` indicates
whether the field's existence condition has predicate conjuncts beyond
the discriminant equality. When there is no residual the case body is
a single direct Ok() check; when there is a residual the body falls
back to the has_${field}() accessor, which encapsulates the full
existence check including the residual conjuncts. The C++ compiler is
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

@robrussell robrussell Jul 22, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

residuals

predicate conjuncts (IR sub-expressions) that the field's existence
condition carries *beyond* the discriminant equality that routed it to
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*

@robrussell robrussell Jul 22, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

(Lever A). This is sound because every switch reaching this point has its
discriminant Known inside the `case K:` label (provably, or via the
emitted `if (!discrim.Known()) return false;` guard), so within the arm
the discriminant equality is already established and `has_${field}() <=>
residual`. Gating on the residual therefore avoids re-reading and
re-comparing the discriminant that `has_${field}()` would recompute.

The gate is required, not just an optimization: `${field}()` re-checks
`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

@robrussell robrussell Jul 22, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"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.

check it replaces.
"""
parts = []
for field, has_residual in entries:
for field, residual in entries:

@robrussell robrussell Jul 22, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

residuals

name = _cpp_field_name(field.name.name.text)
if has_residual:
if residual:

@robrussell robrussell Jul 22, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
        )
    )

if len(residual) == 1:
residual_expr = residual[0]
else:
# Fold multiple conjuncts into a single boolean AND so we
# render (and share subexpressions for) the residual once.
residual_expr = ir_data.Expression(
function=ir_data.Function(
function=ir_data.FunctionMapping.AND,
args=residual,
),
type=ir_data.ExpressionType(boolean=ir_data.BooleanType()),
)
rendered = _render_expression(
residual_expr, ir, subexpressions=subexpressions
).rendered
parts.append(
" if (!has_{0}().Known()) return false;\n".format(name)
" if (!({0}).Known()) return false;\n".format(rendered)
)
parts.append(
" if (has_{0}().ValueOrDefault() && !{0}().Ok()) return false;\n".format(
name
" if (({0}).ValueOrDefault() && !{1}().Ok()) return false;\n".format(
rendered, name
)
)
else:
parts.append(" if (!{}().Ok()) return false;\n".format(name))
return "".join(parts)


def _emit_switch_block(group):
def _emit_switch_block(group, ir, subexpressions):
"""Emits a complete switch block from a collected switch group.

Performs case-label sorting and identical-body coalescing:
Expand All @@ -1808,7 +1837,7 @@ def _emit_switch_block(group):
body_to_labels = {}
body_first_seen = {}
for case_str, case_entry in group["cases_by_label"].items():
body = _render_case_body(case_entry["entries"])
body = _render_case_body(case_entry["entries"], ir, subexpressions)
body_to_labels.setdefault(body, []).append((case_entry["sort_key"], case_str))
if body not in body_first_seen:
body_first_seen[body] = case_entry["sort_key"]
Expand Down
19 changes: 17 additions & 2 deletions testdata/golden_cpp/condition.emb.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4267,6 +4267,19 @@ class GenericCorrectNestedConditionalView final {
static_cast</**/ ::std::int32_t>(
emboss_reserved_local_ok_subexpr_1.UncheckedRead()))
: ::emboss::support::Maybe</**/ ::std::int32_t>());
const auto emboss_reserved_local_ok_subexpr_3 = xc();
const auto emboss_reserved_local_ok_subexpr_4 =
(emboss_reserved_local_ok_subexpr_3.Ok()
? ::emboss::support::Maybe</**/ ::std::int32_t>(
static_cast</**/ ::std::int32_t>(
emboss_reserved_local_ok_subexpr_3.UncheckedRead()))
: ::emboss::support::Maybe</**/ ::std::int32_t>());
const auto emboss_reserved_local_ok_subexpr_5 =
::emboss::support::Equal</**/ ::std::int32_t, bool, ::std::int32_t,
::std::int32_t>(
emboss_reserved_local_ok_subexpr_4,
::emboss::support::Maybe</**/ ::std::int32_t>(
static_cast</**/ ::std::int32_t>(0LL)));

if (!has_x().Known()) return false;
if (has_x().ValueOrDefault() && !x().Ok()) return false;
Expand All @@ -4290,8 +4303,10 @@ class GenericCorrectNestedConditionalView final {
switch (emboss_reserved_switch_discrim.ValueOrDefault()) {
case static_cast</**/ ::std::int32_t>(0LL):
if (!xc().Ok()) return false;
if (!has_xcc().Known()) return false;
if (has_xcc().ValueOrDefault() && !xcc().Ok()) return false;
if (!(emboss_reserved_local_ok_subexpr_5).Known()) return false;
if ((emboss_reserved_local_ok_subexpr_5).ValueOrDefault() &&
!xcc().Ok())
return false;
break;

default:
Expand Down
Loading
Loading