Merge jf/ssa

[jfecher]

Reverts #222 (which itself was a revert). In other words, this PR has been re-opened so it can be reviewed by @guipublic.

Original PR Description:

This PR contains the changes from

• The Operation refactoring
• The deletion refactoring
• The ArrayId refactoring.

It currently passes all tests in cargo test when using evaluate_main_alt

[guipublic]

There are a lots of good ideas in this refactoring. One thing that I do not like though is to have almost everything inside the operation. It makes things less readable, especially for the CSE. It is better to keep arguments and operator separated, the instruction being the one that combines the two. I suggest to do something like this:
Put the arguments in an InstructionKind enum which will be:
binary => with lhs and rhs
unary => for load, cast, phi and not instructions
multiple => vector for arbitrary number of arguments (call, return, intrinsic, nop)
For jump instructions; I am ok to put block ID into the operator, which makes them unary (jne,jeq) or multiple (jump; no argument)
All the operations should then fit into the unary/binary/multiple pattern except for these exceptions:

• PHI: Due to its particularities, it could be added in this enum to specify the block_args
• Truncate: max_value and bit_size are similar and should belongs to the dataflow analysis pass, which is not performed yet, this is why it was temporary put in the instruction. However, for now I am ok to have a truncate kind with the 3 parameters and also to keep the max size in the sub operator as you did.

n.b. when this PR and the function PR are merged, then we can add an instruction kind for call instructions in order to add the vector of pointers used to reference returned arrays.

[jfecher]

The argument count/types are dependent on the Operation type, so they should be within the Operation enum as well for type safety. I re-looked over the CSE part of this PR but I don't see any changes that would harm readability. There was an addition of an anchor type and the rest of the changes mostly match up with the previous code.

Edit: Cranelift does something similar here: https://docs.rs/cranelift-codegen/latest/cranelift_codegen/ir/instructions/enum.InstructionData.html, though in addition to variants for each instruction, most variants also have an opcode as well. I don't think we need both at least.

[guipublic]

Good luck trying to add numbers with not two of them!
What I mean by this is that if you use an instruction with an incorrect number of operands, it simply won't work, so you are solving a non problem here.
But in any cases, you can add the number of arguments to the operation if you want to and ensure it correspond to the instructionkind.

[jfecher]

I don't see the point in complicating the IR design with a InstructionKind enum which itself seems to solve a non-problem to me of the code being more complex. For this change overall there are net lines removed and the csa example specifically does not seem particularly more complex to me.

The benefits of this approach is that just by reading the data structure you can see exactly what arguments each operation supports, and what is expected. So this has more type safety and serves as documentation. In regards to this solving a non-problem, you are forgetting the bugs that were found and fixed by this PR of lhs and rhs being swapped or the wrong one being used in several locations. That is part of the reasoning for why operations like Store take named arguments rather than positional ones - to further improve clarity and prevent future bugs.

Edit: It may also be the case we want an argument-less Opcode enum in addition to this Operation with arguments - like what cranelift does. I'd have to explore the codebase more to see if this simplifies anything by having a function to map from Operation to Opcode. Currently most places we match on Operation we also want to use its arguments as well.

[guipublic]

I do confirm that the number of arguments is a non-problem and there were no bug due to an instruction having the wrong number of arguments.
Readability and complexity are not easy to assess as what is good in one place can have bad impact in another one.

I wonder if the cranelift approach could suit us both. They have this instructiondata that contains the opcode and the arguments, so they are separated as I want. However, the instructiondata enum is defined for each operation, allowing to define specific instructions precisely. Would that be ok for you?

[jfecher]

That would be ok, the only difference from the approach here is the opcode enum which I don't think we need currently. We could use it for the anchor for cse but I'm not sure where else it'd be useful.

[guipublic]

If we keep the opcode enum and use it in the cse anchor, then I am OK as well.

[guipublic]

Test "6_array" is not passing while it works on master (modulo let mut c = on the first line)

[jfecher]

6_array fails on both master and this PR for me in the frontend with const related errors after the relevant muts are added

[guipublic]

6_array fails on both master and this PR for me in the frontend with const related errors after the relevant muts are added

ah yes, const types should really be removed..
Sorry about that, you can use this as the first line to make it work:
let mut c = z-z+2301 as u32;

[jfecher]

Ok, 6_arrays should be fixed now. It was an issue with evaluate_with not using the evaluate function it was passed for some things and using into_const_value instead.

[guipublic]

I am good with the changes, thank you!