-
Notifications
You must be signed in to change notification settings - Fork 79
RISC-V Opcodes Data Cross-Validation #449
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Before you merge, I ask that you change:
Although, @BrianAnakPintar is a really smart guy. ;-) |
|
@ThinkOpenly Ups! I'll do so :) |
dhower-qc
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Can we move this to
backends/rather thanext/? - Did you mean to include
instr_dict.json? If so, we should figure out how to make sure the checked-in version is always consistent with the state of the database.
|
…he UDB with riscv-opcodes generation process to generate riscv-opcodes outputs. Signed-off-by: Afonso Oliveira <[email protected]>
Signed-off-by: Afonso Oliveira <[email protected]>
…s qc_iu Signed-off-by: Afonso Oliveira <[email protected]>
Signed-off-by: Afonso Oliveira <[email protected]>
2c7c616 to
6fe6eb1
Compare
Signed-off-by: Afonso Oliveira <[email protected]>
|
Cool. Now let's just make sure this gets added to the smoke tests. Currently, that takes two changes:
Now that you've told me there is a tool to run GitHub workflows locally, we should be able to get rid of step 1 eventually... |
|
@dhower-qc This is only generating riscv-opcodes outputs, therefore, shouldn't this be a separate task and not be inside the smoke test? This does not test anything, only generates. Probably I could put it into some |
I will try to run it. I hope it is merged with latest main branch to include all fixes to Xqci/Xqccmp. Now dealing with building environment to work on Spike, will take few days. Would be great to see this pull request in main... |
|
@ayosher Just merged this with main to ensure you have the most recent version. |
Sorry, I meant test:regress 😄 |
|
Should I expect down the road this generation to be integrated in some "do" task? |
That is what I was discussing with Derek. I would say yes, this is an output as many others are; however, I'm not entitled to answer it with certainty by myself. @dhower-qc any thoughts on it? |
|
I tried to generate opcodes for qc_iu/Xqci, faced the following error message: |
|
absolutely. we should have something like |
|
@ayosher riscv-opcodes was not updated in remote branch. My bad. There should not be any errors now. |
…uts YAML_DIR=optional_path Signed-off-by: Afonso Oliveira <[email protected]>
|
Updated Rakefile. Example usage: YAML_DIR is optional and can be left blank to default to normal arch/inst. I'll add to regression test later. |
Tried that. I still have the same issue. Now with ./do way. Successfully processed 157 YAML files |
|
@ayosher sorry for the delay.
I still think its a riscv-opcodes problem, because it seems you can not import a file that did not exist in previous version, please try Still, if it does appear the same error, try If it still does not work, please ping me again and so I can take a deeper look trying to figure it out. |
|
@AFOliveira - now it worked, thank you! |
Great! |
e807e87 to
3e73ada
Compare
Signed-off-by: Afonso Oliveira <[email protected]>
Signed-off-by: Afonso Oliveira <[email protected]>
Signed-off-by: Afonso Oliveira <[email protected]>
1e0754a to
38b6893
Compare
Signed-off-by: Afonso Oliveira <[email protected]>
Signed-off-by: Afonso Oliveira <[email protected]>
…a better way than looking into files Signed-off-by: Afonso Oliveira <[email protected]>
|
FYI, this is close to working, but C extensions has shown to be a pain - there's like 20+ operands that need to be further defined by hand, so expect this to take a while to get to 100%. This is the point we are at:
|
Signed-off-by: Afonso Oliveira <[email protected]>
Signed-off-by: Afonso Oliveira <[email protected]>
Signed-off-by: Afonso Oliveira <[email protected]>
Signed-off-by: Afonso Oliveira <[email protected]>
Signed-off-by: Afonso Oliveira <[email protected]>
Signed-off-by: Afonso Oliveira <[email protected]>
f811829 to
4a2085e
Compare
…t comparison of outputs. Signed-off-by: Afonso Oliveira <[email protected]>
…-opcodes. Signed-off-by: Afonso Oliveira <[email protected]>
…f errors when UDB has more info than riscv-opcodes. Signed-off-by: Afonso Oliveira <[email protected]>
|
@AFOliveira, how does this PR relate to #553 and #526 ? |
Well, unfortunately, it does not at all. I realized that this task was way to challenging for a "sole" reason: RISC-V opcodes variable naming is some sort of black magic that is almost impossible to solve. This PR serves to show that we are in a comparable state in terms of instructions to what riscv-opcodes has, but I don't think we should integrate it into any workflow, since that there is no clean way reproducing that .json and leaves this code as a really nice spaghetti mess that we don't want to deal with in the future |
|
Ok, should we close this then, or is there a value to keeping it open for the time being? |
|
Closing it for now |
I integrated @BrianAnakPintar with normal UDB directory for this. I also added the generation of riscv-opcodes outputs, such as encodings.out.h that is what @ayosher was looking for if I am not in error. I added a README.md for detailed instructions.
@ayosher Until next meeting I won't have enough bandwidth to test this with SPIKE and Qualcomm new extensions, but to do so, you should just go to the folder
/ext/opcodes_makerand runmake YAML_DIR=../../cfgs/qc_iu/arch_overlay/inst/and look foroutput/encoding.out.has an input for SPIKE.