Re: [PATCH v2 1/4] x86: zap value-less Disp8MemShift from non-EVEX templates

[Jan Beulich <jbeulich@suse.com>]

On 24.04.2024 08:49, Cui, Lili wrote:
>> In order to allow to continue to use templatized SSE2AVX templates when
>> enhancing those to also cover eGPR usage, Disp8MemShift wants using to
>> deviate from what general template attributes supply. That requires using
>> Disp8MemShift in a way also affecting non-EVEX templates, yet having this
>> attribute set would so far implicitly mean EVEX encoding.
>> Recognize the case and instead zap the attribute if no other attribute
>> indicates EVEX encoding.
>>
> 
> I'm confused about this patch, is it related to the movsd template? You removed the "Masking" for it and only left Disp8MemShift, but I thought it still belongs to EVEX template. 
> 
> +movsd, 0xf210, AVX512F, D|Modrm|EVexLIG|Space0F|VexW1|Disp8MemShift=3|NoSuf|SSE2AVX, { Qword|Unspecified|BaseIndex, RegXMM }

There's no "masking" in an SSE2AVX template. Use of masking in a legacy-form
input instruction is simply wrong, and wants rejecting.

>> --- a/opcodes/i386-gen.c
>> +++ b/opcodes/i386-gen.c
>> @@ -1126,6 +1126,7 @@ process_i386_opcode_modifier (FILE *tabl
>>  			      char **opnd, int lineno, bool rex2_disallowed)  {
>>    char *str, *next, *last;
>> +  bool disp8_shift_derived = false;
>>    bitfield modifiers [ARRAY_SIZE (opcode_modifiers)];
>>    static const char *const spaces[] = {  #define SPACE(n) [SPACE_##n] = #n
>> @@ -1190,7 +1191,10 @@ process_i386_opcode_modifier (FILE *tabl
>>  	      if (strcasecmp(str, "Broadcast") == 0)
>>  		val = get_element_size (opnd, lineno) + BYTE_BROADCAST;
>>  	      else if (strcasecmp(str, "Disp8MemShift") == 0)
>> -		val = get_element_size (opnd, lineno);
>> +		{
>> +		  val = get_element_size (opnd, lineno);
>> +		  disp8_shift_derived = true;
>> +		}
>>
>>  	      set_bitfield (str, modifiers, val, ARRAY_SIZE (modifiers),
>>  			    lineno);
>> @@ -1243,13 +1247,21 @@ process_i386_opcode_modifier (FILE *tabl
>>
>>    /* Rather than evaluating multiple conditions at runtime to determine
>>       whether an EVEX encoding is being dealt with, derive that information
>> -     right here.  A missing EVex attribute means "dynamic".  */
>> -  if (!modifiers[EVex].value
>> -      && (modifiers[Disp8MemShift].value
>> -	  || modifiers[Broadcast].value
>> +     right here.  A missing EVex attribute means "dynamic".  There's one
>> +     exception though: A value-less Disp8MemShift needs zapping rather than
>> +     respecting if no other attribute indicates EVEX encoding.  This is for
>> +     certain SSE2AVX templatized templates to work reasonably.  */  if
>> + (!modifiers[EVex].value)
>> +    {
>> +      if (modifiers[Broadcast].value
>>  	  || modifiers[Masking].value
>> -	  || modifiers[SAE].value))
>> -    modifiers[EVex].value = EVEXDYN;
>> +	  || modifiers[SAE].value)
>> +	modifiers[EVex].value = EVEXDYN;
>> +      else if (disp8_shift_derived)
>> +	modifiers[Disp8MemShift].value = 0;
>> +      else if (modifiers[Disp8MemShift].value)
>> +	modifiers[EVex].value = EVEXDYN;
>> +    }
>>
>  
> Why not just delete the Disp8MemShift? Maybe I missed something.
> 
>   if (!modifiers[EVex].value
>       && (modifiers[Broadcast].value
>           || modifiers[Masking].value
>           || modifiers[SAE].value))
>     modifiers[EVex].value = EVEXDYN;

There are templates where Disp8MemShift is the only indication of EVEX
encoding, e.g. VMOVNTDQ and VMOVNTDQA. Those could all gain EVexDYN (I
think), but that would be a somewhat more intrusive change then. I'm
willing to be convinced of going that route (assuming it actually
would work out).

Jan