Re: [PATCH v3 7/9] Support APX Push2/Pop2

[Jan Beulich <jbeulich@suse.com>]

On 15.12.2023 09:38, Cui, Lili wrote:
>> On 24.11.2023 08:02, Cui, Lili wrote:
>>> @@ -5422,6 +5427,9 @@ md_assemble (char *line)
>>>  	case unsupported_rc_sae:
>>>  	  err_msg = _("unsupported static rounding/sae");
>>>  	  break;
>>> +	case unsupported_rsp_register:
>>> +	  err_msg = _("cannot be used with %rsp register");
>>> +	  break;
>>
>> While this wording looks okay as visible here, please consider it in the context
>> it is used in: "cannot be used with %rsp register for `push2'"
>> is, I'm sorry to say that, clumsy at best. If you want to stick to setting err_msg,
>> how about "%rsp register cannot be used"? Personally I'd prefer a resulting
>> output of "%rsp register cannot be used with `push2'", but I wouldn't insist on
>> you going that route if you don't like that.
> 
> "%rsp register cannot be used" ,this is much better, thanks.

It occurs to me only now (sorry) that %rsp is inappropriate to use
when assembling Intel syntax insns. In that case the % may not be there
(and as a result the entire register name then wants putting in single
quotes, as we do elsewhere).

>>> +check_APX_operands (const insn_template *t) {
>>> +  /* Push2* and Pop2* cannot use RSP and Pop2* cannot pop two same
>> registers.
>>> +   */
>>> +  if (t->mnem_off == MN_push2 || t->mnem_off == MN_push2p
>>> +      || t->mnem_off == MN_pop2 || t->mnem_off == MN_pop2p)
>>
>> Considering (perhaps just theoretical) further additions here, did you consider
>> using switch()? Even without further additions this would imo be more legible
>> (due to there being slightly less redundancy).
>>
> 
> Done.
> 
>   /* Push2* and Pop2* cannot use RSP and Pop2* cannot pop two same registers.
>    */
>   switch (t->mnem_off)
>     {
>     case MN_pop2:
>     case MN_pop2p:
>       if (register_number (i.op[0].regs) == register_number (i.op[1].regs))
>         {
>           i.error = invalid_dest_register_set;
>           return 1;
>         }

Fall-through comment here please, ...

>     case MN_push2:
>     case MN_push2p:
>       if (register_number (i.op[0].regs) == 4
>           || register_number (i.op[1].regs) == 4)
>         {
>           i.error = unsupported_rsp_register;
>           return 1;
>         }

... and break here please.

Jan