Problems with `ldi reg 0`

[sparr]

I should preface this by saying that I haven't built your computer yet, but I want to, either in a simulator or on breadboards, and I prefer to dive into the repository and code before getting physical. So, I'm sorry if some (or, hopefully not, all) of my "will" and "should" below are inaccurate or off base.

ldi {dst: reg} 0 => 0b11 @ 0b01 @ 0b00 @ dst[1:0]

This seems to convert ldi $a 0 into 0b11 01 00 00 which microcode.txt tells me is equivalent to sub $a $a instead of the move 0 $a that I would otherwise expect. While that instruction will produce the clearly desired effect of putting 0 into the a register, I think there are a few problems with this:

1. sub sets flags while move does not. That means ldi $a 0 will set the zero flag, but ldi $a 1 will not clear the zero flag. As a programmer, this is not behavior I would expect or desire.
2. ldi $b 0 is converted to 0b11 01 00 01 which is sub $a $b. Should the third crumb 0b00 here be another dst[1:0] instead so that this will produce 0b11 01 01 01 or sub $b $b?
3. Both sub reg reg and move imm reg take 4 steps. This optimization saves a byte of code, but costs some power activating a bunch of extra control lines and circuits.
4. dst[1:0] truncates the dst parameter to 2 bits, so ldi $sp 0 is accepted by the assembler and produces identical machine code to ldi $a 0, and ditto for $pc/$b and $out/$c. This line should include { assert(reg <= 4) ... } or equivalent.
   This concern also applies to {op: alu_op} {operand_1: reg} {operand_2: reg} => 0b11 @ op[1:0] @ operand_1[1:0] @ operand_2[1:0]
5. No example programs use this pattern for any register other than a. The test suite program doesn't use it at all. Every separate line in the assembler definition probably deserves at lease one test case.

I hope at least some of this makes sense and is helpful.

[vascofazza]

I did not check carefully but I guess you are right about the flag being set due to ldi $a 0 because of its implementation through the sub opcode.

There is actually a good reason why this is implemented this way. Therefore, I would say this is a design choice; the ALU flags should be considered valid only after a cmp or sub operation.
You could still reserve a whole set of opcodes for that operation and "solve" the issue this way.

1. That might indeed be true. I did not check the exact behavior anyway. refer to what is being said above.
2. The instructions are correct. All ALU ops first operand is always $a, therefore the syntax is sub $a $x $dest -> for any register the operation results in $a - $a -> $dest.
3. This is not actually true. sub reg reg is a 1-byte instruction, move imm reg is a 2-byte instruction, therefore it requires an extra fetch cycle to complete.
4. Again, I didn't check the details, this sounds a bit awkward. The microcode has all the possible combinations, in case there's an error feel free to open a PR :) The assertion seems safe. https://github.com/vascofazza/8bit-cpu/blob/master/MK1_CPU/code/microcode.txt
5. The original testing program was more exhaustive, I had to reduce it to make it fit in memory. A second test suite would make sense.

I hope this helps :)

[sparr]

1. Thanks for the clarification. I suspect if I was going to continue less-fork-edly from your design I'd try to contribute a bit of documentation for that sort of thing. As it is I might not stay close enough for it to be relevant, but if I do then I will.
2. That makes perfect sense, and I suspect I'd have figured it out when I built the ALU to play with.
3. I completely forgot about the extra fetch cycle.
4. I found a better way than an assertion. I'll PR something like this after Update cpu instruction definition and asm programs for customasm v0.11 #5 is merged:

#subruledef reg_gp
{
    $a   => 0
    $b   => 1
    $c   => 2
    $d   => 3
}

#subruledef reg_all
{
    {dst: reg_gp} => dst
    $sp  => 4
    $pc  => 5
    $out => 6
}

5. If I stay even close to instruction compatible with yours, I'll try to PR more tests.

I'll go ahead and close this since the major concerns aren't valid, and the fix for item 4 is going to touch a lot more than just this instruction anyway.

PS: I have a very unrealistic new concern... Implementing ldi 0 differently from other ldi imm could expose cryptographic implementations on the cpu to timing attacks :)