Re: [PATCH v3 8/9] Support APX NDD optimized encoding.

[Jan Beulich <jbeulich@suse.com>]

On 12.12.2023 04:18, Hu, Lin1 wrote:
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@suse.com>
>> Sent: Monday, December 11, 2023 8:28 PM
>>
>> On 24.11.2023 08:02, Cui, Lili wrote:
>>> @@ -7675,6 +7727,61 @@ match_template (char mnem_suffix)
>>>  	  i.memshift = memshift;
>>>  	}
>>>
>>> +      /* If we can optimize a NDD insn to legacy insn, like
>>> +	 add %r16, %r8, %r8 -> add %r16, %r8,
>>> +	 add  %r8, %r16, %r8 -> add %r16, %r8, then rematch template.
>>> +	 Note that the semantics have not been changed.  */
>>> +      if (optimize
>>> +	  && !i.no_optimize
>>> +	  && i.vec_encoding != vex_encoding_evex
>>> +	  && t + 1 < current_templates->end
>>> +	  && !t[1].opcode_modifier.evex
>>> +	  && t[1].opcode_space <= SPACE_0F38
>>> +	  && t->opcode_modifier.vexvvvv == VexVVVV_DST)
>>> +	{
>>> +	  unsigned int match_dest_op = can_convert_NDD_to_legacy (t);
>>> +	  size_match = true;
>>
>> This would perhaps better ...
>>
>>> +	  if (match_dest_op != (unsigned int) ~0)
>>> +	    {
>>
>> ... live here
>>
> 
> OK.
>  
>>
>>> +	      /* We ensure that the next template has the same input
>>> +		 operands as the original matching template by the first
>>> +		 opernd (ATT), thus avoiding the error caused by the wrong
>> order
>>> +		 of insns in i386.tbl.  */
>>
>> I'm sorry, but I (still) can't make sense of this last part of the comment, after the
>> comma.
>>
> 
> I mean if someone support new NDD insns and put it in the wrong position, so the part will try to avoid to optimize the insn.

If this is about hypothetical new templates, that would want saying so in
the comment. Thus clarifying that there's no functional effect right now.
I wonder what H.J.'s view on such effectively dead code is.

However, there's a bigger problem with this patch as I realized only a
few minutes ago when looking into Lili's reply on the NDD patch thread:
NDD insns are implicitly zero-upper. Hence converting NDD to legacy insns
needs to be limited to 32- and 64-bit operand size. For 8- and 16-bit
operand size the results would differ, which isn't acceptable under any
-O<n>. It may be okay to do such a conversion even for the smaller sizes,
but then under a separate option explicitly permitting such a functional
difference.

Jan