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

[Jan Beulich <jbeulich@suse.com>]

On 23.10.2023 04:57, Hu, Lin1 wrote:
> Thanks for your reviews, I have responded to your comments. The new patch will be attached to a subsequent email.

Up-front remark: Your way of replying makes it hard to spot what your
responses actually are. Many mail programs allow you to properly prefix
(by convention using '>') responded-to mail contents, it's usually
merely a matter of enabling that mode of operation.

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com> 
> Sent: Thursday, September 28, 2023 5:30 PM
> 
> On 19.09.2023 17:25, Cui, Lili wrote:
>> --- a/gas/config/tc-i386.c
>> +++ b/gas/config/tc-i386.c
>> @@ -7091,6 +7091,46 @@ check_EgprOperands (const insn_template *t)
>>    return 0;
>>  }
>>  
>> +/* Optimize APX NDD insns to non-NDD insns.  */
>> +
>> +static int
> 
> "bool" please when the function merely returns a yes/no indicator.
> 
> * Have modified.
> 
>> +optimize_NDD_to_nonNDD (const insn_template *t) {
>> +  if (t->opcode_modifier.vexvvvv
>> +      && t->opcode_space == SPACE_EVEXMAP4
>> +      && i.reg_operands >= 2
> 
> See the remark near the bottom of the changes to this file: This condition is likely insufficient, as
> - further insns allowing ND may not be treated this way (CCMPscc,
>   CTESTscc, and one of the CFCMOVcc forms at the very least),
> - {nf} uses will want excluding, as it would be merely a waste of
>   time to try to re-match with fewer operands.
> 
> * CCMPSCC and CTESTSCC’s vexvvvv will be false. I think one of the CFCMOVCC forms is same.

By "is same" do you mean "fits the optimization pattern here"?

>> @@ -7562,6 +7602,15 @@ match_template (char mnem_suffix)
>>  	     slip through to break.  */
>>  	}
>>  
>> +      /* If we can optimize a NDD insn to non-NDD insn, like
>> +	 add %r16, %r8, %r8 -> add %r16, %r8, then rematch template.  */
>> +      if (optimize_NDD_to_nonNDD (t))
> 
> I don't think such an optimization should be done without any form of -O.
> 
> As to the function name, maybe better optimize_NDD_to_REX2()?
> 
> * Refer to the optimization of VOP, temporarily set to O1 will be optimized.  If we use 32bit register, some instructions will be optimized from NDD to rex or legacy. Like cmovg  0x90909090(%eax),%edx,%edx, imul   %rdx,%rax,%rdx in our test.

What you you mean by saying "temporarily"?

>> --- /dev/null
>> +++ b/gas/testsuite/gas/i386/x86-64-apx-ndd-optimize.s
>> @@ -0,0 +1,115 @@
>> +# Check 64bit APX NDD instructions with optimized encoding
>> +
>> +	.allow_index_reg
>> +	.text
>> +_start:
>> +inc    %ax,%ax
>> +inc    %eax,%eax
>> +inc    %rax,%rax
>> +inc    %r16,%r16
>> +add    %r31b,%r8b,%r8b
>> +add    %r31b,%r8b,%r31b
>> +addb    %r31b,%r8b,%r8b
>> +add    %r31,%r8,%r8
>> +addq    %r31,%r8,%r8
>> +add    %r31d,%r8d,%r8d
>> +addl    %r31d,%r8d,%r8d
>> +add    %r31w,%r8w,%r8w
>> +addw    %r31w,%r8w,%r8w
>> +{store} add    %r31,%r8,%r8
>> +{load}  add    %r31,%r8,%r8
>> +add    %r31,(%r8),%r31
>> +add    (%r31),%r8,%r8
>> +add    $0x12344433,%r15,%r15
>> +add    $0xfffffffff4332211,%r8,%r8
>> +dec    %r17,%r17
>> +not    %r17,%r17
>> +neg    %r17,%r17
>> +sub    %r15,%r17,%r17
>> +sub    %r15d,(%r8),%r15d
>> +sub    (%r15,%rax,1),%r16,%r16
>> +sub    $0x1234,%r30,%r30
>> +sbb    %r15,%r17,%r17
>> +sbb    %r15,(%r8),%r15
>> +sbb    (%r15,%rax,1),%r16,%r16
>> +sbb    $0x1234,%r30,%r30
>> +adc    %r15,%r17,%r17
>> +adc    %r15d,(%r8),%r15d
>> +adc    (%r15,%rax,1),%r16,%r16
>> +adc    $0x1234,%r30,%r30
>> +or    %r15,%r17,%r17
>> +or    %r15d,(%r8),%r15d
>> +or    (%r15,%rax,1),%r16,%r16
>> +or    $0x1234,%r30,%r30
>> +xor    %r15,%r17,%r17
>> +xor    %r15d,(%r8),%r15d
>> +xor    (%r15,%rax,1),%r16,%r16
>> +xor    $0x1234,%r30,%r30
>> +and    %r15,%r17,%r17
>> +and    %r15d,(%r8),%r15d
>> +and    (%r15,%rax,1),%r16,%r16
>> +and    $0x1234,%r30,%r30
>> +and    $0x1234,%r30
>> +ror    %r31,%r31
>> +rorb   %r31b,%r31b
> 
> Please be consistent with omitting (or having) suffixes (further up you simply test both variants, but that's not the case throughout ...
> 
>> +ror    $0x2,%r12,%r12
>> +rol    %r31,%r31
>> +rolb   %r31b,%r31b
>> +rol    $0x2,%r12,%r12
>> +rcr    %r31,%r31
>> +rcrb   %r31b,%r31b
>> +rcr    $0x2,%r12b,%r12b
>> +rcr    $0x2,%r12,%r12
>> +rcl    %r31,%r31
>> +rclb   %r31b,%r31b
>> +rcl    $0x2,%r12b,%r12b
>> +rcl    $0x2,%r12,%r12
>> +shl    %r31,%r31
>> +shlb   %r31b,%r31b
>> +shl    $0x2,%r12b,%r12b
>> +shl    $0x2,%r12,%r12
>> +sar    %r31,%r31
>> +sarb   %r31b,%r31b
>> +sar    $0x2,%r12b,%r12b
>> +sar    $0x2,%r12,%r12
>> +shl    %r31,%r31
>> +shlb   %r31b,%r31b
>> +shl    $0x2,%r12b,%r12b
>> +shl    $0x2,%r12,%r12
>> +shr    %r31,%r31
>> +shrb   %r31b,%r31b
> 
> ... here.
> 
> * Added suffixes to the command names, I don't understand very well what is suggested inside the parentheses, I just aligned most of the insns' test items.

In parentheses I've merely tried to make more clear what aspects I mean to
be considered for the result being consistent.

Jan