fix: Use get_local_or_global_instruction in try_optimize_array_set_from_previous_get

[aakoshh]

Description

Problem*

Resolves #8199

Summary*

Changes try_optimize_array_set_from_previous_get to use get_local_or_global_instruction.

Additional Context

In this case we are setting the value in a local array but getting the value from a global one, which is why the instruction the value comes from isn't in dfg.

Documentation*

Check one:

• No documentation needed.
• Documentation included in this PR.
• [For Experimental Features] Documentation to be submitted in a separate PR.

PR Checklist*

• I have tested the changes locally.
• I have formatted the changes with Prettier and/or cargo fmt on default settings.

[asterite]

Is there a reason why get_local_or_global_instruction isn't how instructions are fetched by ID by default? This one looks similar to how we sometimes had to call resolve but it wasn't clear when this was needed.

[aakoshh]

@asterite see #8185 (comment)

I looked briefly at instances of dfg[*inst and this one was one of the standouts. The others looked like they weren't dealing with arrays.

[asterite]

LGTM

[jfecher]

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?

[vezenovm]

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.

[jfecher]

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.

[vezenovm]

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.