Refactoring the RISCV architecture to Auto-Sync on LLVM #2756
Conversation
…son, and riscv.h comments for generated content added
arch/RISCV/RISCVInstPrinter.c
Outdated
| SStream_concat0(markup(O, Markup_Register), "x18"); | ||
| } | ||
| break; | ||
| case RISCVZC_RLISTENCODE::RA_S0_S3: |
There was a problem hiding this comment.
These namespace specifiers should have been replaced by AS. This might be a bug.
arch/RISCV/RISCVInstPrinter.c
Outdated
|
|
||
| if (PrintBranchImmAsAddress) { | ||
| uint64_t Target = Address + MCOperand_getImm(MO); | ||
| if (!STI.hasFeature(RISCV_Feature64Bit)) |
There was a problem hiding this comment.
The STI.hasFeature should probably be automated away as well.
You can check the STIFeatureBits patch and just copy and adapt it if you have some time.
arch/RISCV/RISCVInstPrinter.c
Outdated
| if (SysReg && SysReg->haveRequiredFeatures(STI.getFeatureBits())) | ||
| SStream_concat0(markup(O, Markup_Register), SysReg->Name); | ||
| else | ||
| SStream_concat0(markup(O, Markup_Register), formatImm(Imm)); |
There was a problem hiding this comment.
Yeah, better get rid of these formatImm calls. Otherwise we have two places to take care of formatting and everything gets less consistent.
| // which will not print trailing zeros and will use scientific notation | ||
| // if it is shorter than printing as a decimal. The smallest value requires | ||
| // 12 digits of precision including the decimal. | ||
| if (FPVal == (int)(FPVal)) |
There was a problem hiding this comment.
I'd need to check the standard, but casting an float to an integer just rounds it up/down.
Please just use the SStream function for printing it.
…test failures: 1230 out of 4757
|
cc @hainest You are probably aware of this PR, just wanted to ping you as well. Because you mentioned in the Zydis discussion you use Capstone for RISCV as well. |
…ated, it doesn't compile yet but is mostly valid C except for a few problems
| input: | ||
| bytes: [ 0xdb, 0x52, 0x03, 0xa0 ] | ||
| arch: "CS_ARCH_RISCV" | ||
| options: [ "riscv32", "xcvmac" ] |
There was a problem hiding this comment.
| options: [ "riscv32", "xcvmac" ] | |
| options: [ "riscv32", "xcvmac" ] |
These riscv32 should be mapped to CS_MODE_32
| { .str = "riscv32", .val = CS_MODE_RISCV32 }, | ||
| { .str = "riscv64", .val = CS_MODE_RISCV64 }, |
There was a problem hiding this comment.
In response to above. Please stick with the Capstone flag naming convention.
Is RSICV32 is anything else than just 32bit?
| CS_MODE_RISCVFD = 1 << 3, | ||
| CS_MODE_RISCVV = 1 << 4, | ||
| CS_MODE_RISCVZFINX = 1 << 5, | ||
| CS_MODE_RISCVZCMP_ZCMT_ZCE = 1 << 6, |
There was a problem hiding this comment.
I would suggest to add a _ after RISCV for better readability.
CS_MODE_RISCV_FD
| { .str = "riscv64", .val = CS_MODE_RISCV64 }, | ||
| { .str = "v" , .val = CS_MODE_RISCVV }, | ||
| { .str = "zce" , .val = CS_MODE_RISCVZCMP_ZCMT_ZCE}, | ||
| { .str = "zcmp" , .val = CS_MODE_RISCVZCMP_ZCMT_ZCE}, |
There was a problem hiding this comment.
Use CS_MODE_RISCV_ZCMP, CS_MODE_RISCV_F etc.
You can map the feature names passed to llvm-mc via -mattr with the "replace_option_map" object in mcupdater.json.
This way it is replaced automatically when generating.
Your checklist for this pull request
Detailed description
This PR is attempting to refactor the RISCV architecture to be updatable from the Auto-Sync tool, such that it automatically follows RISCV-in-LLVM additions and fixes. Following the refactoring guide
depends-on: capstone-engine/llvm-capstone#83
Test plan
...
Closing issues
...