Re: [PATCH V2] Support APX NF

[Jan Beulich <jbeulich@suse.com>]

On 11.03.2024 14:54, Cui, Lili wrote:
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@suse.com>
>> Sent: Friday, March 8, 2024 5:36 PM
>>
>> On 04.03.2024 09:15, Cui, Lili wrote:
>>> @@ -7218,6 +7229,11 @@ parse_insn (const char *line, char *mnemonic,
>> bool prefix_only)
>>>  		  /* {rex2} */
>>>  		  i.rex2_encoding = true;
>>>  		  break;
>>> +		case Prefix_NF:
>>> +		  /* {nf} */
>>> +		  i.has_nf = true;
>>> +		  i.encoding = encoding_evex;
>>> +		  break;
>>
>> It's not quite as easy, I'm afraid: Have you thought of the "{vex} {nf} ..."
>> case? (I think I previously indicated that their combination, actually in either
>> order, needs properly rejecting.) Without having spent much thought on it,
>> perhaps it would suffice to check here that the field is still encoding_default,
>> and leave the value alone otherwise (in order to reject bad combinations
>> elsewhere).
>>
> Oh, good point. Do you think it's ok to report bad outside of "switch"?
> 
>                 case Prefix_NF:
>                   /* {nf} */
>                   i.has_nf = true;
> -                 i.encoding = encoding_evex;
> +                 if (i.encoding == encoding_default)
> +                   i.encoding = encoding_evex;
>                   break;
>                 case Prefix_NoOptimize:
>                   /* {nooptimize} */
>                   i.no_optimize = true;
>                   break;
>                 default:
>                   abort ();
>                 }
> +             if (i.has_nf && i.encoding != encoding_evex)
> +               {
> +                 as_bad (_("{nf} cannot be combined with {vex}/{vex3}"));
> +                 return NULL;
> +               }

Where the checking is done is secondary to me. My primary point is that
the above isn't correct: There are more encoding variants than just
vex, vex3, and evex.

That said, reporting the error right here may lead to multiple identical
errors being reported for a single line of input. This wants avoiding,
at which point placing the check here is ruled out.

>>> --- a/gas/testsuite/gas/i386/x86-64-apx-evex-promoted-bad.s
>>> +++ b/gas/testsuite/gas/i386/x86-64-apx-evex-promoted-bad.s
>>> @@ -23,7 +23,7 @@ _start:
>>>  	.insn EVEX.L1.66.M12.W0 0x60, %di, %ax
>>>
>>>  	#EVEX_MAP4 movbe %r18w,%ax set EVEX.z == 0b1.
>>> -	.insn EVEX.L0.66.M12.W0 0x60, %di, %ax {%k7}{z}
>>> +	.insn EVEX.L0.66.M12.W0 0x60, %di, %ax {%k3}{z}
>>
>> What's going on here ...
>>
>>> @@ -33,17 +33,25 @@ _start:
>>>  	.insn EVEX.L1.NP.0f38.W1 0xf5, %rax, (%rax,%rbx), %rcx
>>>
>>>  	#EVEX from VEX bzhi %rax,(%rax,%rbx),%rcx EVEX.P[23](EVEX.z) ==
>> 0b1
>>> -	.insn EVEX.L0.NP.0f38.W1 0xf5, %rax, (%rax,%rbx), %rcx {%k7}{z}
>>> +	.insn EVEX.L0.NP.0f38.W1 0xf5, %rax, (%rax,%rbx), %rcx {%k3}{z}
>>
>> ... and here? This is the kind of thing that wants mentioning in the patch
>> description. And once again, especially for larger patches it is generally a bad
>> idea if they come with only legacy ChangeLog entries:
>> These only say "what", never "why". (That said, I think I see why the change is
>> needed.)
> 
> Do you remember that you asked a bad report for illegal use of nf bit?

Yes.

> When we use K7 it sets the nf bit, not wanting to lose their original testing purpose due to the interception of nf, I adjusted its value.

Yes. What's missing is an explanation for the need of this in the patch
description. Without this you made me work out what this change is about,
when otherwise I would have had to merely check that what you say fits
what changes (i.e. me then knowing what exactly to look for).

Jan