diff mbox series

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

Message ID 09611a6a-076f-43b1-986d-16a42a8b50a6@suse.com
State New
Headers
Series RISC-V: assorted fixes and (hopefully) improvements |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_binutils_build--master-aarch64 success Build passed
linaro-tcwg-bot/tcwg_binutils_build--master-arm success Build passed
linaro-tcwg-bot/tcwg_binutils_check--master-aarch64 success Test passed
linaro-tcwg-bot/tcwg_binutils_check--master-arm success Test passed

Commit Message

Jan Beulich May 15, 2026, 1:29 p.m. UTC 
  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.

Comments

Jiawei May 15, 2026, 3:52 p.m. UTC | #1 
> 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>
diff mbox series

Patch

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