-
Notifications
You must be signed in to change notification settings - Fork 44
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
[CHERI-RISC-V] Allow load/store mnemonics without the c prefix #749
[CHERI-RISC-V] Allow load/store mnemonics without the c prefix #749
Conversation
We were not checking capability mode loads/stores, only compressed ones and atomics.
} | ||
|
||
multiclass CLR_r_aq_rl<bits<3> funct3, string opcodestr, | ||
string Namespace = "CapModeOnly_"> { |
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.
Passing the DecoderNamespace is a bit ugly, but I couldn't figure out how to work around InstAlias not having such a field.
7da5124
to
e6a7ccc
Compare
CI is failing because one of the RISC-V tests now gets a different error message - will fix this if the approach is considered ok. |
Instead, make it depend on the type of the operand. This commit only touches the basic load/store instruction without the assembler aliases or compressed instructions.
As a follow-up to the last commit this includes the aliases without an immediate offset.
This adds the atomic operations
This adds support for compressed instructions. Does not yet include Zcb or floating-point loads/stores.
This adds the floating point loads and stores and should conclude this series of patches.
e6a7ccc
to
ef56748
Compare
@@ -22,7 +22,7 @@ la x1, %hi(foo) # CHECK: :[[@LINE]]:8: error: operand must be a bare symbol name | |||
la x1, %lo(foo) # CHECK: :[[@LINE]]:8: error: operand must be a bare symbol name | |||
|
|||
sw a2, %hi(a_symbol), a3 # CHECK: :[[@LINE]]:8: error: operand must be a symbol with %lo/%pcrel_lo/%tprel_lo modifier or an integer in the range [-2048, 2047] | |||
sw a2, %lo(a_symbol), a3 # CHECK: :[[@LINE]]:23: error: invalid operand for instruction | |||
sw a2, %lo(a_symbol), a3 # CHECK: :[[@LINE]]:23: error: operand must be a bare symbol name |
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.
Hm, this is a bit weird, though so is upstream's diagnostics?
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.
Yeah I don't quite understand why patch 1 changes the resulting error, but since neither of them are particularly helpful I don't think it matters.
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.
The approach generally makes sense to me, though I don't love the strip-all-leading-c's approach in the tests... not that I have a better suggestion
I agree this is rather gross and could be fragile but duplicating all the lines seems even more ugly. I did the duplication approach for tests with only a handful of checks but for the larger ones the |
Instead, make it depend on the type of the operand. This does not yet include all instructions, but the initial set of loads/atomics has been converted (the ones that should be part of the standard version of CHERI-RISC-V).