[PATCH][v5] Support Intel USER_MSR

[Jan Beulich]

On 27.10.2023 11:00, Hu, Lin1 wrote:
> @@ -2504,7 +2505,10 @@ smallest_imm_type (offsetT num)
>  	t.bitfield.imm8 = 1;
>        t.bitfield.imm8s = 1;
>        t.bitfield.imm16 = 1;
> -      t.bitfield.imm32 = 1;
> +      if (flag_code != CODE_64BIT)
> +	t.bitfield.imm32 = 1;
> +      else if (fits_in_unsigned_long (num))
> +	t.bitfield.imm32 = 1;
>        t.bitfield.imm32s = 1;
>      }

      if (flag_code != CODE_64BIT || fits_in_unsigned_long (num))
	t.bitfield.imm32 = 1;

would be more clear imo (same further down then).

> @@ -5158,6 +5169,7 @@ md_assemble (char *line)
>    const char *end, *pass1_mnem = NULL;
>    enum i386_error pass1_err = 0;
>    const insn_template *t;
> +  int op;

I know there are lots of example like this, but this new variable would
benefit from being "unsigned int" and from being declared in the narrow-
most scope possible. Then again, why don't you re-use j below?

> @@ -5232,8 +5244,23 @@ md_assemble (char *line)
>        && (t->mnem_off == MN_extrq || t->mnem_off == MN_insertq))
>        swap_2_operands (0, 1);

Did you, btw, consider extending this to cater for UWRMSR as well, in
place of the new construct at the bottom of match_template()? (I'm
not saying it needs to be that way; if you decide to leave things as
is, I may later see about folding both.)

> +  /* For USER_MSR instructions, imm32 stands for the name of an model specific
> +     register (MSR). So we need to avoid set i.types[op].bitfield.imm32 = 0,
> +     when guess_suffix == QWORD_MNEM_SUFFIX. For time-saving reasons,
> +     we specialize the USER_MSR and make only the necessary judgments.  */

This comment wants to move ...

>    if (i.imm_operands)
> -    optimize_imm ();
> +    {
> +      if (!is_cpu(current_templates->start, CpuUSER_MSR))

... right above this if(). I'd also like to suggest rewording, as emphasis
ought to be on 32-bit immediates normally being imm32s for 64-bit operand-
size insns. Mentioning guess_suffix == QWORD_MNEM_SUFFIX would make sense
only when also saying what function this is talking about. Overall maybe:

"For USER_MSR instructions, imm32 stands for the name of an model specific
 register (MSR).  That's an unsigned quantity, whereas all other insns with
 32-bit immediate and 64-bit operand size use sign-extended immediates
 (imm32s).  Therefore these insns are special-cased, bypassing the normal
 handling of immediates here."

> +	optimize_imm ();
> +      else
> +	{
> +	  for (op = i.operands; --op >= 0;)

To accommodate the request further up this will need adjustment. It's not
quite clear anyway why the loop here works downwards.

Unless you think any of the remaining adjustment requests are unclear or
undesirable, okay with those adjustments.

Jan