Fix the encoding and assembly of vsetvl instruction
#359
Conversation
|
Fixing encodings should not be a 178-line diff. If you want to refactor because you think it’s cleaner, fine, but please separate that from the function change that is fixing the encoding. |
The issue is because the encoding and assembly definitions of I don't think the change of refactoring can be separated from fixing the encoding, because the purpose of this refactoring is to fix the encoding issue, and the issue is fixed by refactoring the code of |
|
If the commit message of "fix the encoding" causes misunderstanding, maybe I can modify that description to be more reasonable. |
|
So you're not just changing the encoding and assembly. You're also rewriting their implementations to be functionally different to before, regardless of how the instructions were encoded. |
|
I would adjust the commit message to explain what you wrote in your comment above, as it provides the reasoning for the change. Maybe also add a pointer to #337? In general I think we should be proactive in getting vector issues resolved as they are discovered. I actually think the new version with four simpler instructions w/ helper functions is clearer than the original with the branching logic in each function, so this change looks good to me. |
…iscv#337 The issue shows that the encoding and assembly definitions of vsetvl and vsetvli are incorrectly mixed together, but they actually differ a lot and therefore cannot have a unified definition as the old implementation did, so this section has been rewritten to split their definitions and executions. And because these instructions have similar functional behavior after the decoding stage, some common code is further extracted to be helper functions in this commit.
|
@vmoya-smd, can you review this proposed patch for us? |
|
Other than some personal nitpicking on some comparison, where I would prefer to use parenthesis for clarity even if not required, I think the changes are good. The new code is more clear and avoids code replication between the opcodes compared with just a simple fix of the previous implementation. That justifies the refactoring. |
billmcspadden-riscv
added
the
tgmm-agenda
|
I agree. Sail was chosen in large part for its readability, and this
improves it, especially for those not familiar with its typing system and
precedence
- and those are exactly the people we should ensure can understand the code.
…On Mon, Mar 25, 2024 at 5:51 AM Victor Moya ***@***.***> wrote:
Other than some personal nitpicking on some comparison, where I would
prefer to use parenthesis even it not required I think the changes are good.
The new code is more clear and avoid replication of code compared with
just a simple fix of the previous implementation so I think the refactoring
is appropriate.
—
Reply to this email directly, view it on GitHub
<#359 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AHPXVJQ3ZSLHXSCKAA7RO2TY2AMTJAVCNFSM6AAAAAA7WF2HAKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMJXHEZTEMZSHA>
.
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
|
|
@billmcspadden-riscv, what is our plan to complete this work? |
Signed-off-by: Xinlai Wan <[email protected]>
|
I have resolved the conflicts with the base branch and hope this will make code review easier. |
The implementation of
vsetvlandvsetvliare rearranged, and some helper functions are added.This PR fixed #337 .