Skip to content
This repository has been archived by the owner on Aug 17, 2022. It is now read-only.

Conversation

Nelson1225
Copy link
Collaborator

@Nelson1225 Nelson1225 commented Dec 24, 2019

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).

@jim-wilson
Copy link
Collaborator

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.

Copy link
Collaborator

@jim-wilson jim-wilson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be better with .option support to turn it on and off.

gas/config/tc-riscv.c Show resolved Hide resolved
@aswaterman
Copy link
Contributor

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.

@Nelson1225
Copy link
Collaborator Author

Nelson1225 commented Dec 25, 2019

@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 :)

@Nelson1225 Nelson1225 force-pushed the riscv-binutils-2.33.1-rvv-0.8.x-constraint branch 2 times, most recently from b6e5baa to f4175d8 Compare December 25, 2019 10:31
@Nelson1225
Copy link
Collaborator Author

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.

  1. 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.

  2. 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.

@Nelson1225 Nelson1225 force-pushed the riscv-binutils-2.33.1-rvv-0.8.x-constraint branch from f4175d8 to b274438 Compare December 26, 2019 01:18
@Nelson1225
Copy link
Collaborator Author

Unified the logic of match_widen* and match_narrow* functions. Fixed some typo.

include/opcode/riscv.h Outdated Show resolved Hide resolved
opcodes/riscv-dis.c Outdated Show resolved Hide resolved
opcodes/riscv-opc.c Outdated Show resolved Hide resolved
opcodes/riscv-opc.c Outdated Show resolved Hide resolved
Since the constraint checking may break the hardware exception test cases,
we should add new GAS options (-m[no-]check-constraints) and .option
(.option [no]checkconstraints) to enable/disable the checking.  Disable the
checking in assembler by default, compiler and user can use the new options
to enable it.  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.

1. `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.

2. `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.
@Nelson1225 Nelson1225 force-pushed the riscv-binutils-2.33.1-rvv-0.8.x-constraint branch 2 times, most recently from 33efff3 to edaee3b Compare December 26, 2019 06:02
@Nelson1225
Copy link
Collaborator Author

Thanks you very much @kito-cheng :)
Fixed the typo. Changed the return type of match_func and constraints flag to bfd_boolean.

opcodes/riscv-opc.c Outdated Show resolved Hide resolved
opcodes/riscv-opc.c Outdated Show resolved Hide resolved
opcodes/riscv-opc.c Outdated Show resolved Hide resolved
opcodes/riscv-opc.c Outdated Show resolved Hide resolved
opcodes/riscv-opc.c Outdated Show resolved Hide resolved
opcodes/riscv-opc.c Outdated Show resolved Hide resolved
@Nelson1225 Nelson1225 force-pushed the riscv-binutils-2.33.1-rvv-0.8.x-constraint branch from edaee3b to b86c723 Compare December 27, 2019 02:19
@Nelson1225
Copy link
Collaborator Author

The related support in GCC riscvarchive/riscv-gcc#173

Copy link
Collaborator

@jim-wilson jim-wilson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The .option checkconstraints looks a little awkward to me, but isn't wrong. I think we painted ourselves in a corner by having for instance norelax instead of no-relax at the beginning. We also have the csr checking patch posted upstream and this should ideally be consistent with that one with the naming scheme. We can always add extra .option names later though.

Otherwise it looks good.

@kito-cheng kito-cheng merged commit 587c4ed into riscvarchive:riscv-binutils-2.33.1-rvv-0.8.x Feb 19, 2020
@Nelson1225
Copy link
Collaborator Author

Thank you very much, Jim and Kito. Thanks for the review and approval :)

@Nelson1225
Copy link
Collaborator Author

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 :)

@aswaterman
Copy link
Contributor

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.

@Nelson1225 Nelson1225 deleted the riscv-binutils-2.33.1-rvv-0.8.x-constraint branch February 20, 2020 01:10
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants