Skip to content

fix: Use get_local_or_global_instruction in try_optimize_array_set_from_previous_get #8200

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

Merged
aakoshh merged 3 commits into from
Apr 24, 2025
Merged

fix: Use get_local_or_global_instruction in try_optimize_array_set_from_previous_get #8200

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3f74a69
Add test to show the crash
aakoshh Apr 24, 2025
b254a30
Use DataFlowGraph::get_local_or_global_instruction in try_optimize_ar…
aakoshh Apr 24, 2025
7604e77
nargo fmt
aakoshh Apr 24, 2025
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
23 changes: 10 additions & 13 deletions compiler/noirc_evaluator/src/ssa/ir/dfg/simplify.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -372,20 +372,17 @@ fn try_optimize_array_set_from_previous_get(
target_index: ValueId,
target_value: ValueId,
) -> SimplifyResult {
let array_from_get = match &dfg[target_value] {
Value::Instruction { instruction, .. } => match &dfg[*instruction] {
Copy link
Copy Markdown
Contributor

@jfecher jfecher Apr 24, 2025

Choose a reason for hiding this comment

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

This worries me that we could have code elsewhere using dfg[global_instruction_id] and it'd be silently resolving to the wrong instruction. @vezenovm do you know of anything we could do to help prevent this in the future?

Copy link
Copy Markdown
Contributor

@vezenovm vezenovm Apr 24, 2025

Choose a reason for hiding this comment

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

It is basically to use get_local_or_global_instruction by default for fetching instructions. I didn't want to make the change everywhere originally as it should only affect arrays, but I obviously missed a couple spots. See #8185 (comment) for some more info. Perhaps we should just lean into using get_local_or_global_instruction for all fetching of instructions not just array instructions.

Copy link
Copy Markdown
Contributor

@jfecher jfecher Apr 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I wonder if we could make the indices used by the global DFG invalid for other dfgs?
E.g. fill all those indices with Instruction::Noop.
I'm worried about code written like this in the future as well.

Copy link
Copy Markdown
Contributor

@vezenovm vezenovm Apr 24, 2025

Choose a reason for hiding this comment

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

I wonder if we could make the indices used by the global DFG invalid for other dfgs?
E.g. fill all those indices with Instruction::Noop.

I had thought about something like this as well when originally making globals. Wouldn't Noop be removed upon simplifying which may eventually still lead to overlapping IDs? I think we would need a persistent placeholder like Value::Global that would be used in a similar manner. At the time I thought the trade-off would be extra instructions duplicated across function DFGs. This would theoretically not be too bad but could lead to a compilation memory increase for programs with lots of array globals. Also a compilation time cost from having to iterate (and skip) those filled instructions.

The only time we can access a global instruction is when looking at an array value or iterating the globals graph directly. Iterating over the blocks instructions should always not give us any global instructions. That is why I ultimately chose to lean into only relying on Value::Global and get_local_or_global_instruction.

I do agree though that filling the instructions would be the safest to prevent this code being written in the future. I'm not sure how else we can guarantee it as we still need to be able to index the DFG by InstructionId.

Instruction::ArrayGet { array, index } => {
if *array == array_id && *index == target_index {
// If array and index match from the value, we can immediately simplify
return SimplifyResult::SimplifiedTo(array_id);
} else if *index == target_index {
*array
} else {
return SimplifyResult::None;
}
let array_from_get = match dfg.get_local_or_global_instruction(target_value) {
Some(Instruction::ArrayGet { array, index }) => {
if *array == array_id && *index == target_index {
// If array and index match from the value, we can immediately simplify
return SimplifyResult::SimplifiedTo(array_id);
} else if *index == target_index {
*array
} else {
return SimplifyResult::None;
}
_ => return SimplifyResult::None,
},
}
_ => return SimplifyResult::None,
};

Expand Down
6 changes: 6 additions & 0 deletions test_programs/compile_success_no_bug/regression_8199/Nargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
[package]
name = "regression_8199"
type = "bin"
authors = [""]

[dependencies]
6 changes: 6 additions & 0 deletions test_programs/compile_success_no_bug/regression_8199/src/main.nr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
global G_C: [[str<0>; 4]; 4] =
[["", "", "", ""], ["", "", "", ""], ["", "", "", ""], ["", "", "", ""]];
unconstrained fn main(a: [[str<0>; 4]; 4]) {
let mut f = a;
f[0] = G_C[3];
}
Loading