Add MakeArray instruction to the ssa refactor

[jfecher]

Problem

Several recent issues (#1688, #1698) have been a result of ValueIds being nested inside Values (e.g. Value::Array). This necessitated a more-complex, recursive get_value/resolve function which must also now require a mutable context to insert any new arrays required. This necessitated more cloning which is otherwise unnecessary.

Happy Case

We could make a rule going forward that new nested value ids cannot appear without an instruction that introduced them first. This is for the benefit of passes which map instructions (and thus their value results) to new instructions in order. These passes would be able to continue looking instruction by instruction without needing to recursively peek into the values it references.

Current example:

v6 = call array_hash([v0, v1, v2, v3, v4])

New example:

v5 = make_array [v0, v1, v2, v3, v4]
v6 = call array_hash(v5)

To keep simplification on array instructions working, we will need to do one of:

1. When a make_array instruction is created, always call set_value on its result to set it to a Value::Array instead of the default Value::Instruction.
2. Update get_array_constant to also check for Value::Instructions which refer to a make_array instruction.

Alternatives Considered

No response

Additional Context

No response

Would you like to submit a PR for this Issue?

No

Support Needs

No response

[kevaundray]

@jfecher now that the SSA has matured further, what are your thoughts on this issue?

[jfecher]

@kevaundray we haven't had an issue arise from this in a while so changing this may not fix any bugs but it's something we could explore to see if it cleans up the code any.

[kevaundray]

Okay I'll leave it open and we can review EOY as we'd have even more experience then

[jfecher]

I'm inclined to try adding this again with the goal of removing Value::Array.

The main issue with Value::Array is that it is the only Value which itself contains nested ValueIds, and this necessitates special handling. Anywhere we map value ids for example, we must also check if the value they refer to is a Value::Array because if so we'll need to map its nested ids as well. This came up again in the mem2reg changes (#2463) in which to handle arrays properly we have to recurse through all their elements to e.g. mark them used manually so that any references they refer to are not removed while the array is still present.

The main difficulty with removing Value::Array is answering the question: "what do instructions like array_set optimize to if not Value::Array?" They could optimize to the instruction id of a make_array instruction instead, but this implies every time we insert a new array we'd have to insert a new make_array instruction instead of optimizing out the array_set entirely. This is still workable though, since the extra make_arrays can still be optimized out during dead instruction elimination.

[jfecher]

Closing this after the experiment in #2494 wasn't a large code improvement.

[jfecher]

Reopening this issue since although there was no large code improvement in that unfinished PR, I think it still think it simplifies the mental model of SSA a bit to have no nested ValueIds.