Implementing Shift instructions

[jorbush]

Implementing Shift Instructions

Overview

This PR introduces the implementation and comprehensive testing for four bitwise shift and rotate operations: Shl (Shift Left), Shr (Shift Right), Rol (Rotate Left), and Ror (Rotate Right).

Progress and Testing Details

Each instruction (Shl, Shr, Rol, Ror) has the following:

• Implementation (shift.rs): Defines the operation's behavior, either shifting or rotating bits.
• Tests (integration_test.rs):
  • test_<op>_asm(): Verifies basic functionality using assembly code.
  • test_<op>_stack(): Checks the operation with stack-based addressing.
  • test_<op>_conditional_eq_set(): Confirms the operation sets the EQ flag.
  • test_<op>_set_eq_flag(): Ensures the EQ flag is set correctly for edge cases where the result is zero.
  • test_<op>_asm_greater_than_256(): Checks what happens when shifting with amounts greater than 256.

Conclusion

This PR ensures that the shift and rotate operations (Shl, Shr, Rol, Ror) are implemented and tested across various scenarios.

[jorbush]

For ease of review, these are the specifications:

Shl (Shift Left)

• Syntax:
  • shl in1, in2, out
  • shl.s in1, in2, out, to set swap modifier.
  • shl! in1, in2, out, to set set flags modifier.
  • shl.s! in1, in2, out, to set both swap and set flags modifiers.
• Semantic:
  • Result is computed as in1 << (in2 % 256).
  • Flags are computed as follows:
    • EQ is set if result = 0.
    • Other flags are reset.

Shr (Shift Right)

• Syntax:
  • shr in1, in2, out
  • shr.s in1, in2, out, to set swap modifier.
  • shr! in1, in2, out, to set set flags modifier.
  • shr.s! in1, in2, out, to set both swap and set flags modifiers.
• Semantic:
  • Result is computed as in1 >> (in2 % 256).
  • Flags are computed as follows:
    • EQ is set if result = 0.
    • Other flags are reset.

Rol (Rotate Left)

• Syntax:
  • rol in1, in2, out
  • rol.s in1, in2, out, to set swap modifier.
  • rol! in1, in2, out, to set set flags modifier.
  • rol.s! in1, in2, out, to set both swap and set flags modifiers.
• Semantic:
  • Result is computed as in1 <<< (in2 % 256).
  • Flags are computed as follows:
    • EQ is set if result = 0.
    • Other flags are reset.

Ror (Rotate Right)

• Syntax:
  • ror in1, in2, out
  • ror.s in1, in2, out, to set swap modifier.
  • ror! in1, in2, out, to set set flags modifier.
  • ror.s! in1, in2, out, to set both swap and set flags modifiers.
• Semantic:
  • Result is computed as in1 >>> (in2 % 256).
  • Flags are computed as follows:
    • EQ is set if result = 0.
    • Other flags are reset.

[gianbelinche]

LGTM!

[fkrause98]

Overall, it looks good, let's discus a bit more some minor things and we're done.

[fkrause98]

Not saying this is wrong per se, but I'm a bit confused.

The spec states that you take the lowest byte of the second input, here, you're taking the lowest 4 bytes, are we sure we aren't allowing something that shouldn't work based on the spec? We can also check how the fast vm is doing this to be completely sure.

Plus, I think the usize cast is redundant, why can't it simply be a u32?

[jorbush]

I checked fastVM and if we want something similar, we can use something like this instead:

let shift = src1.low_u32() as u8;

You are right that I get the last 4 bytes, but then I do the mod 256 and I get the last byte. I've done that so it's more aligned with the spec:

- The result is computed as in1 << (in2 % 256).

But I don't know the performance or readability of the code in this case, should I change it to the above alternative?

[jorbush]

I removed the redundant usize, you were absolutely right, thanks!

[fkrause98]

But I don't know the performance or readability of the code in this case, should I change it to the above alternative?

As a rule of thumb, let's focus on simplicity+readability more than performance for now 🙂.

- The result is computed as in1 << (in2 % 256)
I'm a bit confused by the spec, it states you take the lowest byte as binary digits, so the max amount of shifts you can do
is 255, which is the range for numbers modulo 256. So the in2 % 256 looks a bit redundant.
But for now, I think following the spec as is is the best thing to do, so let's only take the second argument modulo 256,
and ignore the low 4 bytes, as it seems redundant.
Let's double check this with more people though, pinging @jrchatruc @IAvecilla @gianbelinche @juan518munoz.
Let's also have tests checking what happens when shifting with amounts greater than 256.

[gianbelinche]

I think both src1.low_u32() and src1 % 256 would work.
I don't think the spec is redundant, since it asumes that in2 is a U256, so doing in2 % 256 is actually the way of getting the last byte