Prototype for ahead-of-time decoding #519
Conversation
matthiasgoergens
changed the title
matthiasgoergens
force-pushed
the
matthias/actually-decode
branch
from
729ee01
to
52b99b3
Compare
| { | ||
| let rd = rd.unwrap_or_else(|| T::from(DecodedInstruction::RD_NULL)); | ||
| InsnRecord([pc, opcode, rd, funct3, rs1, rs2, imm_internal]) | ||
| pub fn new(pc: T, kind: T, rd: T, rs1: T, rs2: T, imm: T) -> Self { |
There was a problem hiding this comment.
Merging opcode+funct3 into kind makes a lot of sense! Can you make a PR just for that?
There was a problem hiding this comment.
In principle there could have been circuits that handle multiple kinds with a funct3 witness, but we are not doing that so it’s ok to remove it.
There was a problem hiding this comment.
Thank. Yes, I would advise against having such circuits. Just have a 'kind' field, and do all the opcode + funct3 + etc merging ahead of time when decoding, so that your circuits don't have to worry.
Merging
opcode+funct3intokindmakes a lot of sense! Can you make a PR just for that?
I prepared one earlier #475
Specifically that's a prototype for how we can use structs with named fields instead of indices into an anonymous slice. I'd like to get that one in, and then make the merge of opcode+funct3 on top of that saner foundation.
The crux of that PR is to use repr(C) to get the struct laid out in order in memory, so we can treat it as a slice, when we need to. (But that trick is not strictly necessary, it just makes things easier to type out.)
There was a problem hiding this comment.
I hoped that could be a simple PR? No need for a complete rewrite.
There was a problem hiding this comment.
By itself that would be a set of reasonably simple PRs. I would have just hoped to do that after the refactor away from an anonymous slice towards named entries.
But I guess I can bite the bullet and deal with the merge conflicts on my side.
| @@ -110,6 +105,37 @@ pub struct DecodedInstruction { | |||
| opcode: u32, | |||
| } | |||
|
|
|||
| #[derive(Clone, Copy, Debug)] | |||
| pub struct DecodedInstruction { | |||
| pub kind: InsnKind, | |||
There was a problem hiding this comment.
Strong personal aversion to this style. It’s a better practice to keep representation details private and expose public methods.
There was a problem hiding this comment.
That's exactly what we are doing here.
The representation that we keep private is incoming u32 word for the instruction.
Our public method just has a slightly more sophisticated return type than just a single number: we return essentially a tuple. We make this more readable by using a struct with named fields instead of a literal tuple.
(Think of it like returning a Result or an Option where you can also access the data type directly, because that is the interface.)
And to keep things even more simple we toss out the initials representation, and only keep what the method returned.
There was a problem hiding this comment.
That being said, if you really feel the need to emulate Java and slap a getter in front of everything, that's not a hill I need to die on for this specific use case.
(In general, pattern matching and borrow checking et al don't work well with the getters-style. So I would suggest to avoid it when possible, and only use it strictly when necessary; instead of reaching for it as your default style in a language like Rust.)
There was a problem hiding this comment.
you really feel the need to emulate Java and slap a getter in front of everything
I missed that one. That is not true and that would be silly.
There can be data containers (pub fields), in which case the module commits to the representation as simple & stable. That means other modules can write the fields directly with low risk of misunderstandings. If over time it becomes more complex then it's time to switch. Rarely the other way.
Yes it could have been that way. Whoever decided against at the time. It could be that way again if you want to spend the time.
|
Looks like this ain't gonna happen. Oh, well. |
A prototype to show how this could look like. It doesn't pass the tests, yet.