[02/19] RISC-V: fold redundant code in riscv_ip()
Jan Beulich
jbeulich@suse.com
Sun May 3 08:47:26 GMT 2026
On 28.04.2026 03:52, Jiawei wrote:
>> The parsing of 'F' and 'O' .insn operands is pretty redundant. Have only a
>> single instance each of common code, with the inner switch()es merely
>> handling the actual value insertion. This in particular simplifies the
>> addition of new sub-forms.
>> ---
>> Since it's entirely internal, can't we rename O4 to either O7 or
>> (describing merely the non-fixed bits) O5? I wonder why O4 was used in the
>> first place ...
>>
>
> The refactoring looks reasonable overall, but I have one question about the
> O2 case.
>
> Before this change, O2 rejected values >= 3, so the accepted range was
> 0...2. With the new common check based on 1U << regno, O2 now accepts
> 0...3.
>
> Is this intended? If O2 is meant to describe a two-bit field, then the new
> range looks natural, but the old code seemed to deliberately reject 3.
> If this is a behavior change, maybe it should be mentioned in the commit
> message or covered by a testcase.
No, I will need to correct that. Thanks for spotting.
Jan
> The O4 part looks equivalent to me, apart from the split diagnostics. I also
> agree that O4 is a misleading internal name; O7 would make the field width
> clearer if we decide to rename it in a follow-up.
>
> Best regards,
>
> Jiawei
>
>
>>
>> --- a/gas/config/tc-riscv.c
>> +++ b/gas/config/tc-riscv.c
>> @@ -3193,72 +3193,47 @@ riscv_ip (char *str, struct riscv_cl_ins
>> INSERT_OPERAND (CRS2, *ip, regno);
>> continue;
>> case 'F':
>> - switch (*++oparg)
>> + if (!ISDIGIT (*++oparg))
>> + goto unknown_riscv_ip_operand;
>> +
>> + /* (Ab)use "regno" here. */
>> + regno = *oparg - '0';
>> + if (my_getSmallExpression (imm_expr, imm_reloc, asarg, p)
>> + || imm_expr->X_op != O_constant
>> + || imm_expr->X_add_number < 0
>> + || imm_expr->X_add_number >= (1U << regno))
>> + {
>> + as_bad (_("bad value for compressed funct%u "
>> + "field, value must be 0...%u"),
>> + regno, (1U << regno) - 1);
>> + break;
>> + }
>> +
>> + switch (regno)
>> {
>> - case '6':
>> - if (my_getSmallExpression (imm_expr, imm_reloc, asarg, p)
>> - || imm_expr->X_op != O_constant
>> - || imm_expr->X_add_number < 0
>> - || imm_expr->X_add_number >= 64)
>> - {
>> - as_bad (_("bad value for compressed funct6 "
>> - "field, value must be 0...63"));
>> - break;
>> - }
>> - INSERT_OPERAND (CFUNCT6, *ip, imm_expr->X_add_number);
>> - imm_expr->X_op = O_absent;
>> - asarg = expr_parse_end;
>> - continue;
>> -
>> - case '4':
>> - if (my_getSmallExpression (imm_expr, imm_reloc, asarg, p)
>> - || imm_expr->X_op != O_constant
>> - || imm_expr->X_add_number < 0
>> - || imm_expr->X_add_number >= 16)
>> - {
>> - as_bad (_("bad value for compressed funct4 "
>> - "field, value must be 0...15"));
>> - break;
>> - }
>> - INSERT_OPERAND (CFUNCT4, *ip, imm_expr->X_add_number);
>> - imm_expr->X_op = O_absent;
>> - asarg = expr_parse_end;
>> - continue;
>> -
>> - case '3':
>> - if (my_getSmallExpression (imm_expr, imm_reloc, asarg, p)
>> - || imm_expr->X_op != O_constant
>> - || imm_expr->X_add_number < 0
>> - || imm_expr->X_add_number >= 8)
>> - {
>> - as_bad (_("bad value for compressed funct3 "
>> - "field, value must be 0...7"));
>> - break;
>> - }
>> - INSERT_OPERAND (CFUNCT3, *ip, imm_expr->X_add_number);
>> - imm_expr->X_op = O_absent;
>> - asarg = expr_parse_end;
>> - continue;
>> -
>> - case '2':
>> - if (my_getSmallExpression (imm_expr, imm_reloc, asarg, p)
>> - || imm_expr->X_op != O_constant
>> - || imm_expr->X_add_number < 0
>> - || imm_expr->X_add_number >= 4)
>> - {
>> - as_bad (_("bad value for compressed funct2 "
>> - "field, value must be 0...3"));
>> - break;
>> - }
>> - INSERT_OPERAND (CFUNCT2, *ip, imm_expr->X_add_number);
>> - imm_expr->X_op = O_absent;
>> - asarg = expr_parse_end;
>> - continue;
>> + case 6:
>> + INSERT_OPERAND (CFUNCT6, *ip, imm_expr->X_add_number);
>> + break;
>> +
>> + case 4:
>> + INSERT_OPERAND (CFUNCT4, *ip, imm_expr->X_add_number);
>> + break;
>> +
>> + case 3:
>> + INSERT_OPERAND (CFUNCT3, *ip, imm_expr->X_add_number);
>> + break;
>> +
>> + case 2:
>> + INSERT_OPERAND (CFUNCT2, *ip, imm_expr->X_add_number);
>> + break;
>>
>> - default:
>> - goto unknown_riscv_ip_operand;
>> + default:
>> + goto unknown_riscv_ip_operand;
>> }
>> - break;
>> +
>> + imm_expr->X_op = O_absent;
>> + asarg = expr_parse_end;
>> + continue;
>>
>> default:
>> goto unknown_riscv_ip_operand;
>> @@ -3709,97 +3684,87 @@ riscv_ip (char *str, struct riscv_cl_ins
>> continue;
>>
>> case 'O':
>> - switch (*++oparg)
>> + if (!ISDIGIT (*++oparg))
>> + goto unknown_riscv_ip_operand;
>> +
>> + /* (Ab)use "regno" here. */
>> + regno = *oparg - '0';
>> + /* O4 is a misnomer, really describing a 7-bit field. */
>> + if (regno == 4)
>> + regno += 3;
>> + if (my_getOpcodeExpression (imm_expr, imm_reloc, asarg)
>> + || imm_expr->X_op != O_constant
>> + || imm_expr->X_add_number < 0
>> + || imm_expr->X_add_number >= (1U << regno))
>> + {
>> + as_bad (_("bad value for opcode field, "
>> + "value must be 0...%u"),
>> + (1U << regno) - 1);
>> + break;
>> + }
>> +
>> + if (regno > 2 && (imm_expr->X_add_number & 3) != 3)
>> + {
>> + as_bad (_("bad value for opcode field, "
>> + "lower 2 bits must be 0x3"));
>> + break;
>> + }
>> +
>> + switch (*oparg)
>> {
>> case '4':
>> - if (my_getOpcodeExpression (imm_expr, imm_reloc, asarg)
>> - || imm_expr->X_op != O_constant
>> - || imm_expr->X_add_number < 0
>> - || imm_expr->X_add_number >= 128
>> - || (imm_expr->X_add_number & 0x3) != 3)
>> - {
>> - as_bad (_("bad value for opcode field, "
>> - "value must be 0...127 and "
>> - "lower 2 bits must be 0x3"));
>> - break;
>> - }
>> INSERT_OPERAND (OP, *ip, imm_expr->X_add_number);
>> - imm_expr->X_op = O_absent;
>> - asarg = expr_parse_end;
>> - continue;
>> + break;
>>
>> case '2':
>> - if (my_getOpcodeExpression (imm_expr, imm_reloc, asarg)
>> - || imm_expr->X_op != O_constant
>> - || imm_expr->X_add_number < 0
>> - || imm_expr->X_add_number >= 3)
>> - {
>> - as_bad (_("bad value for opcode field, "
>> - "value must be 0...2"));
>> - break;
>> - }
>> INSERT_OPERAND (OP2, *ip, imm_expr->X_add_number);
>> - imm_expr->X_op = O_absent;
>> - asarg = expr_parse_end;
>> - continue;
>> + break;
>>
>> default:
>> goto unknown_riscv_ip_operand;
>> }
>> - break;
>> +
>> + imm_expr->X_op = O_absent;
>> + asarg = expr_parse_end;
>> + continue;
>>
>> case 'F':
>> - switch (*++oparg)
>> + if (!ISDIGIT (*++oparg))
>> + goto unknown_riscv_ip_operand;
>> +
>> + /* (Ab)use "regno" here. */
>> + regno = *oparg - '0';
>> + if (my_getSmallExpression (imm_expr, imm_reloc, asarg, p)
>> + || imm_expr->X_op != O_constant
>> + || imm_expr->X_add_number < 0
>> + || imm_expr->X_add_number >= (1U << regno))
>> {
>> - case '7':
>> - if (my_getSmallExpression (imm_expr, imm_reloc, asarg, p)
>> - || imm_expr->X_op != O_constant
>> - || imm_expr->X_add_number < 0
>> - || imm_expr->X_add_number >= 128)
>> - {
>> - as_bad (_("bad value for funct7 field, "
>> - "value must be 0...127"));
>> - break;
>> - }
>> + as_bad (_("bad value for funct%u field, "
>> + "value must be 0...%u"),
>> + regno, (1U << regno) - 1);
>> + break;
>> + }
>> + switch (regno)
>> + {
>> + case 7:
>> INSERT_OPERAND (FUNCT7, *ip, imm_expr->X_add_number);
>> - imm_expr->X_op = O_absent;
>> - asarg = expr_parse_end;
>> - continue;
>> + break;
>>
>> - case '3':
>> - if (my_getSmallExpression (imm_expr, imm_reloc, asarg, p)
>> - || imm_expr->X_op != O_constant
>> - || imm_expr->X_add_number < 0
>> - || imm_expr->X_add_number >= 8)
>> - {
>> - as_bad (_("bad value for funct3 field, "
>> - "value must be 0...7"));
>> - break;
>> - }
>> + case 3:
>> INSERT_OPERAND (FUNCT3, *ip, imm_expr->X_add_number);
>> - imm_expr->X_op = O_absent;
>> - asarg = expr_parse_end;
>> - continue;
>> + break;
>>
>> - case '2':
>> - if (my_getSmallExpression (imm_expr, imm_reloc, asarg, p)
>> - || imm_expr->X_op != O_constant
>> - || imm_expr->X_add_number < 0
>> - || imm_expr->X_add_number >= 4)
>> - {
>> - as_bad (_("bad value for funct2 field, "
>> - "value must be 0...3"));
>> - break;
>> - }
>> + case 2:
>> INSERT_OPERAND (FUNCT2, *ip, imm_expr->X_add_number);
>> - imm_expr->X_op = O_absent;
>> - asarg = expr_parse_end;
>> - continue;
>> + break;
>>
>> default:
>> goto unknown_riscv_ip_operand;
>> }
>> - break;
>> +
>> + imm_expr->X_op = O_absent;
>> + asarg = expr_parse_end;
>> + continue;
>>
>> case 'y': /* bs immediate */
>> my_getExpression (imm_expr, asarg, force_reloc);
>>
>
More information about the Binutils
mailing list