[PATCH v2 02/16] RISC-V: fold redundant code in riscv_ip()

[Jiawei]

> 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.
> ---
> v2: Fix O2 to permit only values 0...2. Re-base over O4 -> O7 rename.
>      Fold diagnostics in 'O' handling back into being a single one.
LGTM.

Thank for the update, and the O4 -> O7 preparatory rename makes
the shared O handling cleaner.

>
> --- 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;
>   
> -		      default:
> -			goto unknown_riscv_ip_operand;
> +		    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;
>   		    }
> -		  break;
> +
> +		  imm_expr->X_op = O_absent;
> +		  asarg = expr_parse_end;
> +		  continue;
>   
>   		default:
>   		  goto unknown_riscv_ip_operand;
> @@ -3709,97 +3684,80 @@ 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';
> +	      if (my_getOpcodeExpression (imm_expr, imm_reloc, asarg)
> +		  || imm_expr->X_op != O_constant
> +		  || imm_expr->X_add_number < 0
> +		  /* O2 encodes C opcodes, i.e. doesn't permit value 3.  */
> +		  || imm_expr->X_add_number >= (1U << regno) - (regno == 2)
> +		  || (regno > 2 && (imm_expr->X_add_number & 3) != 3))
> +		{
> +		  as_bad (_("bad value for opcode field, "
> +			    "value must be 0...%u%s"),
> +			  (1U << regno) - (regno == 2) - 1,
> +			  regno > 2 ? _(" and lower 2 bits must be 0x3") : "");
> +		  break;
> +		}
> +
> +	      switch (*oparg)
>   		{
>   		case '7':
> -		  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);

Reviewed-by: Jiawei <jiawei@iscas.ac.cn>