chore(ssa refactor): Fix loading from mutable parameters

[jfecher]

Related issue(s)

Resolves #

Description

Summary of changes

Fixes a couple bugs with mutable parameters:

1. Mutable parameters were not being Instruction::Load'ed from automatically because they were not wrapped in Value::Mutable. This lead to a later panic in mutate_load_into_store.
2. Because we evaluate lhs before rhs in lhs = rhs; we generate a load from lhs before we evaluate rhs and store to it. To create the store however, we replaced the load with a store previously, which would then create a store instruction storing rhs before rhs was even evaluated. So the IR would resemble store v3 at v1; v3 = .... Since the store should be after everything else, I've replaced mutate_load_into_store with a new function to instead remove the old load and insert a new store at the end of the current block.
3. Fixed a small issue where store instructions were being printed backwards.

This PR depends on the previous fixes PR which is in the process of being merged. Until then this PR will show those changes as well.

Dependency additions / changes

Test additions / changes

Checklist

• I have tested the changes locally.
• I have formatted the changes with Prettier and/or cargo fmt with default settings.
• I have linked this PR to the issue(s) that it resolves.
• I have reviewed the changes on GitHub, line by line.
• I have ensured all changes are covered in the description.

Documentation needs

• This PR requires documentation updates when merged.

Additional context

[jfecher]

Update: Fixed storing to mutable arrays as well. Previously the code

fn main() {
    let mut my_array = [1, 2];
    my_array[1] = 3;
}

Would result in the IR:

fn main f0 {
  b0():
    v3 = alloc 2 fields
    store Field 1 at v3
    v5 = add v3, Field 1
    store Field 2 at v5
    v6 = alloc 1 fields
    store v3 at v6
    v9 = load v6
    v12 = mul Field 1, Field 1
    store Field 3 at v9
    return unit 0
}

Note the final v12 that is never used and that we're storing to v9 (the start of the array) rather than (start + 1).
After the newest commit we'll produce the following fixed code. With the correct v13 instruction to get the offset of the array before storing to it.

fn main f0 {
  b0():
    v3 = alloc 2 fields
    store Field 1 at v3
    v5 = add v3, Field 1
    store Field 2 at v5
    v6 = alloc 1 fields
    store v3 at v6
    v9 = load v6
    v12 = mul Field 1, Field 1
    v13 = add v9, v12
    store Field 3 at v13
    return unit 0
}

For the curious the v12 1 * 1 instruction that looks useless is used to calculate type offsets if the element type of the array is larger than 1 field.

Edit: The root cause seems to have actually been a bug in insert_load not inserting the address addition if the offset was not a constant. I've fixed this but kept the previous fix as well since it seems more correct to not insert the loads in the first place rather than removing them later.

[jfecher]

After some more testing I've determined this is the final bugfix PR needed for the new ssa gen pass 🎉. It likely still has bugs but none that I can find by running any of our examples and looking at the output manually.

Note that this is except for one known failure: The first-class-functions test fails completely when we attempt to use a value from another function without importing it as a parameter first. This is by design, but fixing it means we need to do proper closure conversion. Proper closure conversion was needed previously due to a known bug when using closures but for the most part the old SSA ir was fine with using these values without importing them so it worked for the most part. I've opened an appropriate issue for this.

[kevaundray]

After some more testing I've determined this is the final bugfix PR needed for the new ssa gen pass 🎉. It likely still has bugs but none that I can find by running any of our examples and looking at the output manually.

Note that this is except for one known failure: The first-class-functions test fails completely when we attempt to use a value from another function without importing it as a parameter first. This is by design, but fixing it means we need to do proper closure conversion. Proper closure conversion was needed previously due to a known bug when using closures but for the most part the old SSA ir was fine with using these values without importing them so it worked for the most part. I've opened an appropriate issue for this.

Yay :D Great work on getting the ssa gen pass complete!

Can you link the issue for poper closure conversion?

Spoke too soon, its #1254

[kevaundray]

LGTM - one nit but not blocking

[kevaundray]

Would result in the IR:

fn main f0 {
  b0():
    v3 = alloc 2 fields
    store Field 1 at v3
    v5 = add v3, Field 1
    store Field 2 at v5
    v6 = alloc 1 fields
    store v3 at v6
    v9 = load v6
    v12 = mul Field 1, Field 1
    store Field 3 at v9
    return unit 0
}

Sidenote: I love the way blocks/functions are printed now :)