fix(veryl): materialize array literal function arguments - #410
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0f066fb73b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ir_builder.emit(SIRInstruction::Store( | ||
| convert(var_id, WORKING_REGION), |
There was a problem hiding this comment.
When the same array-taking function is invoked inside one of its own call arguments or array-literal elements, both invocations store into the address derived solely from the formal's var_id. For example, while lowering pick('{8'h11, 8'h22}, pick('{8'h00, 8'h01}, 0)), the inner pick overwrites the outer call's materialized pick.x before the outer indexed load, producing 8'h00 instead of 8'h11. The backing address must be unique per active invocation rather than reusing the formal's working-region slot.
Useful? React with 👍 / 👎.
| let mut cached_elements = std::collections::HashMap::new(); | ||
| for linear_index in 0..layout.element_count { |
There was a problem hiding this comment.
Every array-literal argument is now expanded into one store per formal element even when the function only performs a statically resolved access such as x[0]. A large LUT-style formal, especially one called with '{default: value}, therefore creates O(array size) SIR instructions and temporary storage per call instead of lowering only the selected expression as the previous constant-index path did; sufficiently large parameterized arrays can make compilation consume excessive time and memory. Keep the direct selection path for calls whose accesses are statically known, and materialize the complete view only when dynamic or whole-array access requires it.
Useful? React with 👍 / 👎.
| let array_views = self.materialize_function_array_views( | ||
| &bindings, targets, domain, convert, sources, ir_builder, | ||
| )?; |
There was a problem hiding this comment.
When a function assigns an array input wholesale to an array output, such as function copy(x: input logic<8>[2], y: output logic<8>[2]) { y = x; }, this call materializes x once here, but extract_function_target_expr later substitutes the original array-literal binding into y and parse_expression evaluates that literal again. Thus an actual like '{touch(side), 8'h00} executes touch twice and corrupts caller-visible side effects; the output expression should load the already materialized view rather than re-expanding the binding.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1c66f616d0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| SystemFunctionKind::Bits(input) | ||
| | SystemFunctionKind::Size(input) |
There was a problem hiding this comment.
$bits and $size operands
When an array formal is used only by $bits(x) or $size(x), descending into the operand here treats the whole-array reference as needing a view and evaluates/materializes the bound literal. However, parse_system_function_call handles these queries as compile-time constants at lines 1871-1879 and deliberately never evaluates their operands. Consequently, an actual such as '{observe(input, side), default: 0} now executes an otherwise-unevaluated side effect, while large literals also generate unnecessary per-element stores.
Useful? React with 👍 / 👎.
| // Fully static literal element accesses are deliberately omitted and | ||
| // continue through the direct-selection path below. |
There was a problem hiding this comment.
Skipping a view for a fully static access loses the evaluated value if the same formal later needs a dynamic or whole-array view. For example, with y = x[0]; return x[i];, an actual '{observe(input, side), 0} evaluates element 0 directly while propagating y, then materialize_bound_array_literal_view evaluates it again while preparing the return, duplicating the caller-visible side effect. Fresh evidence in the current revision is this mixed static-then-dynamic path, which is not covered by the existing whole-array-output reuse fix.
Useful? React with 👍 / 👎.
|
|
||
| let mut cached_elements = std::collections::HashMap::new(); | ||
| let mut elements = Vec::with_capacity(layout.element_count); | ||
| for linear_index in 0..layout.element_count { |
There was a problem hiding this comment.
This coordinate-order loop can evaluate array-literal expressions in a different order from the literal itself. If a valid pattern places default: before an explicit value, select_array_literal_element still maps the explicit value to index 0, so the explicit expression executes before the syntactically earlier default expression; the ordinary parse_array_literal path instead evaluates items in source order. With output-producing calls as the two expressions, this reverses the final caller-visible side effect, so unique item expressions should be evaluated in source order before their registers are mapped to coordinates.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 729907e42e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| collect(cond, views, seen); | ||
| collect(then_expr, views, seen); | ||
| collect(else_expr, views, seen); |
There was a problem hiding this comment.
When a dynamic array access occurs only in one ternary arm, traversing both arms here causes its bound literal to be materialized before the condition is evaluated. For example, return guard ? x[i] : 0 with guard == 0 still evaluates every expression in an actual such as '{observe(input, side), default: 0}, producing caller-visible side effects from an arm that should not execute; evaluating the view before the condition can also reverse ordering relative to side effects in the condition itself.
Useful? React with 👍 / 👎.
| Factor::FunctionCall(call) => { | ||
| for expr in call.inputs.values() { | ||
| collect(expr, views, seen); | ||
| } |
There was a problem hiding this comment.
Every nested function input is traversed as though the callee needs its value, so forwarding a bound array through a function that only applies $bits or $size still materializes and evaluates the original literal. Fresh evidence beyond the earlier direct $bits case is this new traversal of every nested call input: outer('{observe(input, side), default: 0}), where outer(x) returns inner(x) and inner returns $bits(x), executes observe even though the eventual operand is compile-time-only.
Useful? React with 👍 / 👎.
| let Some(selected_expr) = | ||
| self.select_array_literal_element(items, &coordinates, &array_dims) | ||
| else { | ||
| unreachable!("validated array literal covers every formal element"); |
There was a problem hiding this comment.
For a multidimensional formal, shape validation accepts an array-valued expression for a complete subarray, such as passing '{row0, row1} to x: logic<8>[2, 2]. When a dynamic or whole-array access requests a view, select_array_literal_element cannot descend into either array-valued item and returns None, so this supposedly validated case reaches the unreachable! and panics the compiler instead of lowering the valid argument.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c264ea2954
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| Expression::Binary(lhs, _, rhs, _) => { | ||
| collect(lhs, views, seen); | ||
| collect(rhs, views, seen); |
There was a problem hiding this comment.
For && and ||, this unconditional RHS traversal causes prepare_function_array_views_for_expression to materialize a bound array literal before parse_logic_op evaluates the left operand. Thus guard && (x[i] != 0) with guard == 0 still evaluates side-effecting literal elements even though the RHS block is skipped; logical RHS views need the same branch-local preparation and state handling as ternary arms.
Useful? React with 👍 / 👎.
| // An arm-local view is not initialized on paths that skip that arm, | ||
| // so it must not escape the ternary's merge point. | ||
| self.function_array_view_stack = pre_ternary_array_views; |
There was a problem hiding this comment.
When a dynamic access occurs in a ternary arm in one output expression and a later output or return statically accesses the same formal, resetting the view map here loses the already-evaluated snapshot on the path that took that arm. For example, first = guard ? x[i] : 0; return x[0]; materializes every side-effecting literal item when guard is true, then evaluates item 0 again for the return; fresh evidence in this revision is that invocation-wide preparation analyzes each output/return separately, so it does not prevent this cross-expression duplication.
Useful? React with 👍 / 👎.
| let mut views = Vec::new(); | ||
| self.collect_array_views_for_expression(expr, &mut views, &mut HashSet::default()); | ||
| for var_id in views { | ||
| self.ensure_function_array_view_at( | ||
| frame, var_id, targets, domain, convert, sources, ir_builder, | ||
| )?; |
There was a problem hiding this comment.
This whole-expression prepass evaluates every required array view before normal expression lowering, which reverses caller-visible side-effect order even when no control flow is involved. For example, in observe(1, side) + x[i], with x bound to '{observe(2, side), default: 0}, the literal is materialized first and then the left operand writes 1, whereas the ordinary left-to-right lowering evaluates the left operand before reaching x[i] and should leave side as 2; views must be initialized when their access is evaluated rather than before the surrounding expression.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e84d0c513b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let planned = self.function_array_view_plan_stack[frame].contains(var_id); | ||
| if planned || self.array_access_needs_view(*var_id, var_index, var_select) { | ||
| self.ensure_function_array_view_at( | ||
| frame, *var_id, targets, domain, convert, sources, ir_builder, |
There was a problem hiding this comment.
When a later conditional expression contains a dynamic access, planned also forces an earlier static access to materialize the entire literal. For example, with first = x[0]; return guard ? x[i] : 0, a false guard still causes the first assignment to evaluate every side-effecting item in x, even though only element 0 and neither dynamic arm nor its remaining elements should be evaluated. Fresh evidence in this revision is this invocation-wide promotion of a reached static access, rather than the previously reported eager traversal of the ternary arm; cache the selected static element and defer completion of the view until the dynamic access executes.
Useful? React with 👍 / 👎.
| for view in frame.values() { | ||
| if clobbered.contains(&view.backing_var_id) && restored.insert(view.backing_var_id) | ||
| { | ||
| let layout = self.array_view_layout(view.backing_var_id)?; | ||
| self.store_array_view_elements( |
There was a problem hiding this comment.
When multiple active forwarded formals alias the same backing and control flow initializes only one of them, this loop restores whichever alias the HashMap happens to yield first after a nested invocation clobbers that backing. The unselected alias can carry initialized == false with dummy zero elements; restoring those zeros corrupts the backing while the selected alias remains marked initialized, so a subsequent access skips rematerialization and reads the dummy values. Fresh evidence beyond the existing reentrant-call report is the new branch-merged initialized state: restoration must select a runtime-initialized snapshot (or restore from the owning source view) rather than deduplicating solely by backing ID.
Useful? React with 👍 / 👎.
| let array_view_frame = self.function_arg_stack.len().checked_sub(1); | ||
| let array_view_candidates = self.array_view_merge_candidates([right]); | ||
| let array_view_params = | ||
| self.alloc_array_view_merge_params(&array_view_candidates, ir_builder)?; | ||
| let pre_rhs_array_views = |
There was a problem hiding this comment.
When a short-circuit expression is evaluated while lowering a nested call's array-literal item, the RHS can materialize an array formal belonging to an earlier caller frame, but this code snapshots and merges only the innermost frame. For example, if an inner literal contains guard && (x[i] != 0) where x belongs to the outer function, lowering the RHS leaves the outer frame marked initialized even on the runtime path where guard short-circuits; a later outer x[0] then skips materialization and loads stale backing storage. Fresh evidence beyond the existing direct short-circuit report is this cross-frame state leak; all active frames touched by an arm must be restored and merged at the control-flow join.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8867382d06
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| states.iter().all(|state| { | ||
| state[params.key.frame] | ||
| .get(¶ms.key.var_id) | ||
| .and_then(|view| view.cached_literal_items.get(*key)) | ||
| .is_some_and(|other| other == *value) |
There was a problem hiding this comment.
When a branch performs only a static access such as first = guard ? x[0] : 0, its cache exists only in that branch state, so requiring every predecessor to contain the same cache entry discards it at the merge. A subsequent return x[0] then evaluates the literal item again on the path where guard was true; if that item contains an output-producing call, the caller-visible side effect occurs twice. The merge needs to carry per-item cache initialization conditionally rather than retaining only entries already present on all paths.
Useful? React with 👍 / 👎.
| let needs_actual = if formal.r#type.array.is_empty() { | ||
| input_usage.runtime_reads.contains(arg_id) | ||
| } else { | ||
| input_usage.array_views.contains(arg_id) | ||
| }; |
There was a problem hiding this comment.
When a short-circuit or ternary branch calls a nested function that reads an array formal only at a static index, array_views is empty even though lowering that read populates the caller frame's lazy item cache. Consequently the actual argument is omitted from the merge candidates, the control-flow join restores the pre-branch view stack, and a later access reevaluates the same side-effecting literal item on the path that executed the nested call. Array formals with runtime reads must also have their lazy cache state tracked through the join without forcing a complete backing view.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: caa9e93f91
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for frame in self.function_array_view_stack.iter().rev() { | ||
| for view in frame.values() { |
There was a problem hiding this comment.
When the same array-taking function is nested at least three calls deep, this reverse traversal restores the immediate caller first and then overwrites it with older callers because every invocation shares the formal's backing_var_id. After the innermost call returns, the middle invocation therefore resumes with the outermost array in its backing storage and can return the wrong element. Fresh evidence beyond the existing two-level reentrant case is this depth-three restoration order; restoration should leave the nearest active owning frame's snapshot installed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 541b553079
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if seen.insert(key) { | ||
| views.push(key); | ||
| } |
There was a problem hiding this comment.
When a conditional or short-circuit arm accesses a formal x whose bound array literal contains a dynamic read of an active caller formal y, this collector records only x and never traverses x's binding. Lowering the arm can therefore initialize y, but the join restores the pre-arm view stack and carries only x, so a subsequent y access reevaluates its literal and duplicates caller-visible side effects. For example, an outer function returning middle('{y[index], default: 0}, 0, guard) + y[0], with y bound to '{observe(side + 1, side), default: 0}, evaluates observe twice when guard is true. Fresh evidence after the earlier cross-frame fix is that the current candidate collector follows nested call inputs but stops when it encounters a bound formal variable.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 26c08ee8de
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| && let Some(source_expr) = self.function_arg_stack[source_frame].get(bound_var_id) | ||
| { | ||
| self.collect_array_views_in_bound_expression(source_frame, source_expr, views, seen); |
There was a problem hiding this comment.
When an array formal is bound directly to an active caller formal, this recursion traverses the caller's bound expression but never adds the caller formal itself as a merge candidate. For example, if inner(x, guard) evaluates guard ? x[0] : 0, outer(y, guard) returns inner(y, guard) + y[0], and y is bound to a side-effecting literal, the taken branch caches the item only in y's frame; the inner ternary join carries x but restores y to its pre-branch state, so the later y[0] evaluates that literal item again. Add the directly forwarded source view/cache key before recursively inspecting its binding.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 177970f19b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let elements = (0..layout.element_count) | ||
| .map(|_| { | ||
| if layout.is_2state { | ||
| ir_builder.alloc_bit(layout.element_width, layout.signed) | ||
| } else { |
There was a problem hiding this comment.
For a conditional or short-circuit branch that only reads a static element, such as guard ? x[0] : 0, candidate collection still reaches this loop and allocates one merge register per formal element; array_view_state_args then emits the corresponding dummy values even though no backing view is materialized. A large LUT-style formal therefore produces O(array size) SIR for each control-flow merge and can exhaust compilation time or memory despite the static access being otherwise lazy. Fresh evidence beyond the earlier eager-materialization report is this current control-flow state representation: carry only the accessed item cache unless a complete view actually exists.
Useful? React with 👍 / 👎.
| if needs_actual && let Some(expr) = call.inputs.get(arg_path) { | ||
| collect(expr, views, seen); |
There was a problem hiding this comment.
When a function call in a ternary or short-circuit arm has an indexed output actual such as dst[x[0]], this collector visits only the call inputs even though emit_multi_dst_assign later evaluates the destination index through op_store. If x is an active formal bound to a side-effecting literal, that branch can populate its lazy item cache, but the control-flow merge omits x and restores the pre-branch state; a subsequent x[0] then evaluates the literal item again on the path that executed the call. Include the index and select expressions of every call.outputs destination in the merge candidates.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 603d33dce2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| array_views = self.function_array_view_stack.pop().unwrap(); | ||
| self.function_arg_stack.pop(); |
There was a problem hiding this comment.
When an output RHS has materialized an array view and the corresponding output destination index invokes the same array-taking function, popping both frame stacks here hides the caller snapshot while emit_multi_dst_assign evaluates that index. The nested invocation can therefore overwrite the shared formal working region without restore_active_function_array_views restoring this invocation; a later output or return sees its saved view as initialized and loads the nested call's elements from the stale backing. Keep the frame active through destination evaluation or restore its snapshot immediately afterward.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 99aa6a5255
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if needs_actual && let Some(expr) = call.inputs.get(arg_path) { | ||
| self.collect_function_input_usage(expr, usage, active_calls)?; | ||
| } | ||
| } |
There was a problem hiding this comment.
When a conditional arm calls a helper whose array formal is used only in an output actual's index, this usage analysis classifies the formal as unused because it follows only call.inputs. Lowering the helper still evaluates that destination index and can populate an active caller literal's lazy cache, but the enclosing control-flow merge omits the caller view and later reevaluates the same side-effecting item. Fresh evidence beyond the existing direct-output fix is that the new nested usage analyzer still does not traverse call.outputs; include their index/select expressions here as well.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e7690631cd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ir_builder.seal_block(SIRTerminator::Jump(merge_block, evaluated)); | ||
| ir_builder.switch_to_block(merge_block); |
There was a problem hiding this comment.
When completing a conditionally cached literal item, evaluate_array_literal_item can initialize another active array view, but this join merges only defined_ranges and dynamic_defined_vars; it leaves function_array_view_stack from the evaluation block installed on both successors. For example, after guard ? x[0] : 0 caches an x[0] expression containing y[i], a later dynamic x[j] takes this branch and a subsequent y[0] can use cache registers created only on the !guard path, yielding invalid SSA or stale values when guard was true. Snapshot and merge nested array-view state across this internal branch just as the ternary and short-circuit joins do.
Useful? React with 👍 / 👎.
Summary
Why
A non-variable array argument has no storage address for the normal dynamic-load path. Selecting every possible literal element with a mux would make compile time and runtime scale with the array length for each access.
The formal argument's working region now provides temporary backing storage. Its existing shape and stride metadata drives
SIROffset::Elementloads, keeping each dynamic access independent of array length.This addresses the array-literal function argument portion of #68. The Veryl cross-validation cases remain ignored pending veryl-lang/veryl#3131.
Validation
cargo test -p celox --test flip_flop(256 passed, 55 ignored)cargo test -p celox --test flip_flop function_call(96 passed, 21 ignored)cargo test -p celox-frontend-veryl(65 passed)cargo clippy --locked -p celox --all-targets --no-depscargo fmt --check