Add support for RISC-V Zbb (Bit manipulation) extension #204
Conversation
aman4150
force-pushed
the
riscv-zbb-ext
branch
from
3799560
to
6d277e2
Compare
aman4150
force-pushed
the
riscv-zbb-ext
branch
2 times, most recently
from
f4e0773
to
9190266
Compare
… u16 Signed-off-by: Aman <[email protected]>
Signed-off-by: Aman <[email protected]>
Signed-off-by: Aman <[email protected]>
Note, we are introducing new instructions in our amd64 assembler, because it makes the compiled code small, or well, sometimes it is necessary. Signed-off-by: Aman <[email protected]>
Now that we support 64-bit execution mode in the polkavm, let's also extend our implementation of Zbb instruction set in both interpreter and compiler to support execution of these new instructions in 64-bit mode. Signed-off-by: Aman <[email protected]>
Signed-off-by: Aman <[email protected]>
aman4150
force-pushed
the
riscv-zbb-ext
branch
from
9190266
to
541d1f8
Compare
Signed-off-by: Aman <[email protected]>
aman4150
force-pushed
the
riscv-zbb-ext
branch
from
d1de884
to
8e1cd53
Compare
| @@ -1282,6 +1295,21 @@ pub mod inst { | |||
| None, | |||
| (fmt.write_fmt(core::format_args!("mov {}, {}", self.1.name_from(self.0), self.2.name_from(self.0)))), | |||
|
|
|||
| movsx_8_to_64(Reg, Reg) => | |||
There was a problem hiding this comment.
Can you add this and the rest of new instructions to the tests like you did for rol?
There was a problem hiding this comment.
Sure, not sure how I missed these.
| [I_64, I_32] rotate_left = 152, | ||
| [I_64, I_32] rotate_left_word = 153, |
There was a problem hiding this comment.
Is one supposed to be 32-bit and the other 64-bit? If so why they're both enabled on both 64-bit and 32-bit? The 32-bit shouldn't need the 64-bit one, right?
Also, to keep with the current convention, when there are 32-bit and 64-bit variants of the same instruction could you rename them so that one ends with _32 and the other with _64?
There was a problem hiding this comment.
No, rotate_left and rotate_left_word are available on both 32-bit and 64-bit.
The rotate_left_word is same as rotate_left on 32-bit, but on 64-bit it yield a sign-extended result.
| Self::RotateLeft => { | ||
| if rv64 { | ||
| cast((lhs as u64).rotate_left(rhs as u32)).to_signed() | ||
| } else { | ||
| cast((lhs as u32).rotate_left(rhs as u32) as i32).to_i64_sign_extend() | ||
| } | ||
| }, |
There was a problem hiding this comment.
Okay, so for this instruction specifically I would want to see a separate variant (instead of the rv64 flag) like it is for e.g. addition where you have a dedicated "32 bit + sign extend" variant. Otherwise anywhere else you will want to do any optimizations on these operations you'll have to remember to have this extra if and if you forget it'll be a bug; having this as an extra variant will remove this potential footgutn
There was a problem hiding this comment.
Hm, there is no reason for actually doing sign extension here, I guess I simply copied the statement from above and replaced it with to_i64_sign_extend.
On 32-bit, it will get truncated again, so sign-extending or not, it's the same story.
There was a problem hiding this comment.
I suppose, I can still split the this operation type, but this does seem a bit redundant. Because anywhere we don't extend the result type to match the return value, code would not compile.
I will however, replace to_i64_sign_extend with a simply cast with zero-extend op.
| cast((lhs as u32).rotate_left(rhs as u32) as i32).to_i64_sign_extend() | ||
| } | ||
| }, | ||
| Self::RotateLeftWord => cast((lhs as u32).rotate_left(rhs as u32) as i32).to_i64_sign_extend(), |
There was a problem hiding this comment.
Can you rename this one to RotateLeft32AndSignExtend?
There was a problem hiding this comment.
Sure, we could, but then it conflicts with our convention of using 32 and 64 in the operation kind name.
I suppose, I could rename is to RotateLeftWordSignExtend .
| (O::RotateLeft, lhs, C(0)) => lhs, | ||
| (O::RotateLeft, C(0), _) => C(0), | ||
|
|
||
| (O::RotateLeftWord, lhs, C(0)) => lhs, |
There was a problem hiding this comment.
Isn't this incorrect? :P
(This will truncate the lhs and sign extend it to 64-bit instead of being a nop, no? See, that's the reason I was explicit about 32/64 and 32AndSignExtend everywhere. :P)
There was a problem hiding this comment.
This will truncate the lhs and sign extend it to 64-bit instead of being a nop
And that is right thing to do.
RotateLeftWord is not a 32-bit only instruction, this is what the spec says:
let rs1 = EXTZ(X(rs1)[31..0])
let shamt = X(rs2)[4..0];
let result = (rs1 << shamt) | (rs1 >> (32 - shamt));
X(rd) = EXTS(result[31..0]);
No description provided.