[PATCH 1/8] Support APX GPR32 with rex2 prefix

[Jan Beulich]

On 07.11.2023 09:06, Cui, Lili wrote:
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@suse.com>
>> Sent: Monday, November 6, 2023 11:03 PM
>>
>> On 02.11.2023 12:29, Cui, Lili wrote:
>>> @@ -1119,6 +1148,18 @@ process_i386_opcode_modifier (FILE *table, char
>> *mod, unsigned int space,
>>>  	fprintf (stderr,
>>>  		 "%s: %d: W modifier without Word/Dword/Qword
>> operand(s)\n",
>>>  		 filename, lineno);
>>> +
>>> +      /* The part about judging EVEX encoding should be synchronized with
>>> +	 is_evex_encoding.  */
>>> +      if (modifiers[Vex].value
>>> +	  || ((space > SPACE_0F || has_special_handle)
>>> +	      && !modifiers[EVex].value
>>> +	      && !modifiers[Disp8MemShift].value
>>> +	      && !modifiers[Broadcast].value
>>> +	      && !modifiers[Masking].value
>>> +	      && !modifiers[SAE].value))
>>> +	modifiers[NoEgpr].value = 1;
>>
>> While this is just i386-gen (and hence being somewhat inefficient isn't the end
>> of the world) I still wonder whether we need all the parts of this condition:
>> Do we really need all the constituents of this EVEX related checks? Wouldn't it
>> also help is_evex_encoding() if we switched to uniformly having EVex
>> attributes on all EVEX templates? A presently missing EVex attribute, after all,
>> merely is another way of saying EVexDYN, if I'm not mistaken. (Such an
>> adjustment, if deemed to help, would of course want to come as a separate,
>> prereq patch.)
>>
> 
> Yes, EVex is another way of saying EVexDYN, it should be appear in every EVEX template, when we merge EVex128, EVex256 and EVex512 into one template we omitted the expression of EVexDYN. So some EVEX templates don’t have this tag. If we want to re-add it, we need new values.

I don't understand. When there's (e.g.) EVex128, not EVexDYN should appear at
the same time. Otoh ...

> Such as:
> vcvttps2dq, 0xF35B, AVX512F, Modrm|Masking|Space0F|VexW0|Broadcast|Disp8ShiftVL|CheckOperandSize|NoSuf|SAE, { RegXMM|RegYMM|RegZMM|Dword|Unspecified|BaseIndex, RegXMM|RegYMM|RegZMM }

... aiui this one could have EVexDYN added without change in behavior, but
would then allow being recognized as needed EVEX-encoding by just checking
the .evex field, not any of the other fields is_evex_encoding() presently
needs to check.

>> Furthermore, is this correct at all for mixed VEX/EVEX templates?
>>
> After merging the templates we only have one entry and I prefer to set [NoEgpr].value to 1. Don't check NoEgpr for all EVEX instruction in check_EgprOperands function. 
> 
> check_EgprOperands (const insn_template *t)
>  {
> -  if (t->opcode_modifier.noegpr)
> +  if (t->opcode_modifier.noegpr && !need_evex_encoding())

So why would you add an attribute just to then ignore it by adding extra
code?

>>> --- a/opcodes/i386-opc.tbl
>>> +++ b/opcodes/i386-opc.tbl
>>> @@ -891,7 +891,7 @@ rex.wrxb, 0x4f, x64, NoSuf|IsPrefix, {}
>>> <pseudopfx:ident:cpu, disp8:Disp8:0, disp16:Disp16:0, disp32:Disp32:0, +
>>>                        load:Load:0, store:Store:0, +
>>>                        vex:VEX:0, vex2:VEX:0, vex3:VEX3:0, evex:EVEX:0, +
>>> -                      rex:REX:x64, nooptimize:NoOptimize:0>
>>> +                      rex:REX:x64, rex2:REX2:x64,
>>> + nooptimize:NoOptimize:0>
>>
>> Seeing this I realized that there's something missing here (an APX_F
>> dependency), which then again would not have had an effect without the
>> patch [1] sent earlier today.
>>
>> Jan
>>
>> [1] https://sourceware.org/pipermail/binutils/2023-November/130345.html
> 
> Changed to 
> 
> +#define APX_F_64 APX_F|x64
> +
> <pseudopfx:ident:cpu, disp8:Disp8:0, disp16:Disp16:0, disp32:Disp32:0, +
>                        load:Load:0, store:Store:0, +
>                        vex:VEX:0, vex2:VEX:0, vex3:VEX3:0, evex:EVEX:0, +
> -                      rex:REX:x64, rex2:REX2:x64, nooptimize:NoOptimize:0>
> +                      rex:REX:x64, rex2:REX2:APX_F_64, nooptimize:NoOptimize:0>
> 
> When we have" x86: split insn templates' CPU field" in trunk, I will change it to #define APX_F_64 APX_F&x64.

I've meanwhile put together the Cpu64 patch I was thinking of. No "&x64"
should then be needed anymore for any of the APX templates. Before sending
that one out, I will want to first see whether I can re-order it with the
patch sent earlier, as this would allow that other patch to shrink in
size (fewer "|x64" to convert to "&x64").

Jan