Skip to content

[DSLX] Implement frontend cover() autoinsertion for branches - #4904

Open
ansidu wants to merge 1 commit into
google:mainfrom
ansidu:emit_auto_cover
Open

ansidu wants to merge 1 commit into
google:mainfrom
ansidu:emit_auto_cover

Conversation

@ansidu

@ansidu ansidu commented Sep 3, 2026 •

Copy link
Copy Markdown

Instrument each if/else-if/match arm with a cover op during DSLX -> IR conversion, where the original branch structure is still present. Covers are gated on the fully-nested reachability predicate (outer && inner), producing correct per-arm firing.

Label format <fn>_<branch>_line_<N>_col_<M> (function, branch kind, line/col) to make it easier to find corresponding source location even without debug info (that is also supported). Enabled via --emit_auto_cover=branch (default off). Warns once per function when skipped due to a missing implicit token.

@google-cla

google-cla Bot commented Sep 3, 2026

Copy link
Copy Markdown

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@ansidu

ansidu commented Sep 3, 2026

Copy link
Copy Markdown
Author

My first attempt was to add an IR -> IR pass, but that approach doesn't work: in IR if/else/match are flattened into independent select nodes, losing which arms are nested (example 1) and which are just data-dependent (example 2).Without this information, gating conditions for cover nodes cannot be computed correctly.

Example 1:

    let res = if c2 {
      if c1 { ... } else { ... }
    } else { ... }

Example 2:

    let tmp = if c1 { ... } else { ... }
    let res = if c2 { tmp } else { ... }

@dplassgit
dplassgit self-requested a review September 14, 2026 15:31
std::optional<Span> span = node->GetSpan();
if (span.has_value()) {
return absl::StrFormat(
"%s_%s_line_%d_pos_%d", function_name, then_arm ? "then" : "else",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

_col_ would be more readable than _pos_ since "pos" means "position in the file" rather than "column on this line". Same comment elsewhere in this PR

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

replaced pos with col

@ansidu

ansidu commented Sep 15, 2026

Copy link
Copy Markdown
Author

I want to clarify the behavior of cover-related options. Currently, the --emit_cover option enables or disables cover property generation. I introduced a new option --emit_auto_cover=.... Should --emit_cover also apply to auto-inserted cover nodes?
In other words, should
ir_converter --emit_cover=false --emit_auto_cover=branch
disable the generation of auto-inserted cover nodes?

@ansidu

ansidu commented Sep 16, 2026

Copy link
Copy Markdown
Author

I've updated the patch to properly handle if-else if-else chains. Previously, for an else if two cover nodes were emitted: one for else and one for the nested if (since else if is structurally a nested conditional). I found this rather confusing, so the updated patch emits only one cover node for the nested if, skipping the node for the outer else.

@ansidu
ansidu requested a review from dplassgit September 16, 2026 15:45
@dplassgit

Copy link
Copy Markdown
Contributor

I want to clarify the behavior of cover-related options. Currently, the --emit_cover option enables or disables cover property generation. I introduced a new option --emit_auto_cover=.... Should --emit_cover also apply to auto-inserted cover nodes? In other words, should ir_converter --emit_cover=false --emit_auto_cover=branch disable the generation of auto-inserted cover nodes?

Yes, I think --emit_cover=false should prevent auto-inserted cover nodes.

not_any_prev_selected,
this_arm_selected});
});
if (options_.emit_auto_cover.Contains(AutoCoverKind::kBranch)) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

As mentioned in the main PR discussion, if the auto_cover is explicitly set to false, this should not run. Or you could do it when the flags are parsed.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Done. Added ShouldEmitAutoCover method to ConvertOptions

Instrument each if/else-if/match arm with a cover op during DSLX -> IR
conversion, where the original branch structure is still present. Covers are
gated on the fully-nested reachability predicate (outer && inner), producing
correct per-arm firing.

Label format <fn>_<branch>_line_<N>_col_<M> (function, branch kind, line/col)
to make it easier to find corresponding source location even without debug info
(that is also supported). Enabled via `--emit_auto_cover=branch` (default off).
Warns once per function when skipped due to a missing implicit token.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants