Closed Nelson1225 closed 4 years ago
In another thread, t was Palmer and/or Andrew that mentioned that adding constraint checking to the assembler might break hardware verification tests. I think there are a few things we can do about this. We could have the compiler add the new option by default, so that people using the compiler get the errors by default, but people writing assembly code don't. Or alternatively we can add a .option check (or whatever) to enable the checking which the compiler emits by default. I think this might be better as it is more consistent with the current approach for other stuff like rvc/norvc and pic/nopic. It also lets you turn the checking on/off in the middle of a file if necessary, which might be useful if someone needs an invalid instruction, but wants the rest of it to be valid code.
Something like a .option that the compiler emits by default seems pretty OK to me. Breaking the current tests does not seem acceptable, not just because of the extra support issues that will raise, but also because other people have mimicked the style of those tests, so the problem is more widespread than it might seem.
@jim-wilson @aswaterman, Thank you very much. Things are worse than I thought. I have sent a series of CSR checking commits to upstream, and also meets the same problem as here. Adding GAS options and .option directives to enable/disable these checking is a good solution. We disable these checking in assembler by default, and then enable them by compiler or user adding the new options. It is reasonable to me.
I plan to add both GAS option and .option to control the constraints checking, like the CSR checking. If the constraints checking is disabled, then we always call match_opcode
rather than the specific match_func
to check only the basic encoding. For now I'm going to improve the assembler's error reporting. So after that, we will see what more these constraints options can do :)
Ideally, the match_func
should only need to check the basic encoding if the checking is disabled. But there are several cases may break this rules, since not all match_func
are only used to check the instruction constraints.
match_vs1_eq_vs2
and match_vd_eq_vs1_eq_vs2
with INSN_ALIAS
.
These are used for disassembler. vmcpy.m
and vmand.mm
have the same match and mask macros (MATCH_VMANDMM and MASK_VMANDMM), but use the match_vs1_eq_vs2
to decide which one to disassemble.
match_c_add
, match_c_lui
, match_slli_as_c_slli
and match_srxi_as_c_srxi
.
These are used to avoid converting the RVI instructions to the corresponding hint RVC.
However, I prefer not to change the original behavior of non-vector instructions in this commit and rvv branch. Therefore, the new constraints options can only affect the following match_func
so far,
match_widen_vd_neq_vs1_neq_vs2_neq_vm
,
match_widen_vd_neq_vs1_neq_vm
,
match_widen_vd_neq_vs2_neq_vm
,
match_widen_vd_neq_vm
,
match_quad_vd_neq_vs1_neq_vs2_neq_vm
,
match_quad_vd_neq_vs2_neq_vm
,
match_narrow_vd_neq_vs2
,
match_vd_neq_vs1_neq_vs2_neq_vm
,
match_vd_neq_vs2_neq_vm
,
match_vd_neq_vm
,
match_vmv_nf_rv
Beside, I think use new options to control the match_func
of vwcvt.x.x.v
and vwcvtu.x.x.v
with INSN_ALIAS is safe. Since their corresponding non-alias instructions (vwadd.vx and vwaddu.vx) are also use the same match_func
to check the constraints (match_widen_vd_neq_vs2_neq_vm
), and have different match and mask macros.
Unified the logic of match_widen and match_narrow functions. Fixed some typo.
Thanks you very much @kito-cheng :)
Fixed the typo. Changed the return type of match_func
and constraints
flag to bfd_boolean
.
The related support in GCC https://github.com/riscv/riscv-gcc/pull/173
Thank you very much, Jim and Kito. Thanks for the review and approval :)
I think we painted ourselves in a corner by having for instance norelax instead of no-relax at the beginning.
Yeah, the naming of command line options are fine, but the naming of .option directive are not consistent with the former... It would be good if we have the consistent naming from now, and have some remedies like you said, always add extra .option names later though :)
The norelax
is inherited from the MIPS option name noreorder
. Not the best decision on my part. I guess we could start using hyphens in new names, and for the legacy ones, we can create aliases with hyphens.
I get the request recently that the assembler's constraints checking break hardware exception test cases. As I know, stop to assemble instructions with constraints is reasonable to me. But it is hard to check whether the hardware exceptions work expected for the constraints. For now, the compromises are use the hard cored instruction (.word or .2byte) and .insn directives to build the test cases. But I think the current test cases won't built by them and can not check the exception for constraints.
It seems quite easy to resolve the problem by adding a GAS internal option to disable checking the RVV constraints (always call
match_opcde
to check). But for the normal ISA (scalar), we have much complex parsing method for the constraints (The same opcode name with different operands, Co and Cj, ...). It doesn't make sense to me that we can use a internal option to affect the current operand parsing. Also, add a internal option just used to disable the RVV constraints is hard to maintain.My personal opinion is not to support a internal option used to disable the constraints. But I can not convince them to use the hard-cored instruction and .insn directive to build the test cases in the sort-term. Since the former is hard to be generated automatically, and the later for RVV isn't complete now (the RVV instruction name are still discussed).
Ideally, I suppose our assembler shouldn't allow to generate the instruction with constraints. The internal usage option is also hard to maintain. It would be great if the constraint test cases can be built by the .insn directives. I hope we can have the leeway that not to support the internal usage option, which just for testing, in the long-term. But in the sort-term, it is understandable that the internal option is easy to develop (build the test cases).