cranelift: avoid per-variable value sets when tracking stack-map bindings#13466
Conversation
Move `stack_map_vars` and `stack_map_values` from `FunctionBuilderContext`
into `SSABuilder` itself so that bindings can be recorded as they happen,
in the two places SSA values get bound to a variable:
* `SSABuilder::def_var`, when the user supplies a new definition; and
* `SSABuilder::find_var`, when SSA construction synthesises a block
parameter as the definition.
github-actions
Bot
added
the
cranelift
cfallin
left a comment
cfallin
left a comment
There was a problem hiding this comment.
Thanks -- general shape looks OK, but the API behavior does change per below, so we'll need to be careful.
Marking a variable as needing a stack map now tracks bindings eagerly, so
`declare_var_needs_stack_map` must be called before any `def_var` of the
variable; earlier definitions would be silently omitted from stack maps.
* Document the ordering constraint on `declare_var_needs_stack_map`.
* Assert in `SSABuilder::mark_var_needs_stack_map` that the variable has no
recorded definitions yet, so misuse panics rather than silently dropping a
GC root from stack maps.
* Fix the fuzzer's variable pool, which declared the stack-map need after
the initial `def_var`, dropping that definition from stack maps.
cfallin
left a comment
cfallin
left a comment
There was a problem hiding this comment.
Thanks -- one more change on your latest commit and I think this is good to go.
| &mut self.stack_map_vars | ||
| /// Mark `var` as needing its bindings tracked for stack maps. | ||
| pub fn mark_var_needs_stack_map(&mut self, var: Variable) { | ||
| assert!( |
There was a problem hiding this comment.
Let's make this a debug_assert since it's doing a potentially expensive linear search.
There was a problem hiding this comment.
In the intended call pattern, the variable has no recorded definitions yet, so the scan is O(1). It's only linear in the misuse case, which panics anyway. But I can switch to debug_assert! if that feels safer to you.
There was a problem hiding this comment.
All the same, we try to keep checks like these debug_assert. Please make the change, thanks.
| /// Panics if the variable's type is larger than 16 bytes, if this | ||
| /// variable has not been declared yet, or if it has already been defined. | ||
| /// Panics if the variable's type is larger than 16 bytes or if this | ||
| /// variable has not been declared yet. |
There was a problem hiding this comment.
We shouldn't remove the "panics" comment here: it still panics if it panics in debug-builds.
The check that `mark_var_needs_stack_map` runs before any definition of the variable scans the variable's per-block definitions. Demote it from `assert!` to `debug_assert!` so it does not run in release builds.
2 participants
Follow-up to #13449 addressing review feedback from this comment:
SecondaryMap<Variable, EntitySet<Value>>is dense — O(max value id) bits per variable — and wasteful for the sparse sets that show up in practice.Instead of tracking every SSA value ever bound to a variable and propagating the needs-stack-map bit at finalize time, this PR records values eagerly at the two binding sites —
SSABuilder::def_varandSSABuilder::find_var(the latter when SSA construction synthesizes a block parameter). A new helperrecord_stack_map_binding(var, val)checksstack_map_varsand inserts on a hit; the per-variable set,values_for_var, and the finalize loop all go away.stack_map_varsandstack_map_valuesmove fromFunctionBuilderContextintoSSABuilderso the binding sites can reach them. The publicFunctionBuilderAPI is unchanged.Result:
EntitySet<Value>of values that need stack maps is ever allocated — no per-variable structure.stack_map_vars.contains(var)short-circuits insiderecord_stack_map_bindingand nothing is touched.