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

["Hu, Lin1" <lin1.hu@intel.com>]

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: Monday, October 23, 2023 3:24 PM
> To: Hu, Lin1 <lin1.hu@intel.com>
> Cc: Lu, Hongjiu <hongjiu.lu@intel.com>; binutils@sourceware.org; Cui, Lili
> <lili.cui@intel.com>
> Subject: Re: [PATCH 5/8] Support APX NDD optimized encoding.
> 
> 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"?
> 

Because I didn't find any insn that allowing ND, but its vexvvvv is true in CFCMOVcc's table. I'm going to assume that you found it, but I don't see it. I believe its vexvvvv is false, too. So I say it is same as CCMPSCC and CTESTSCC.

> >> @@ -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"?

Since we don't have any relevant experience, we'd like to see if you have any opinion on this. If you haven't, users will use O1 to optimize their NDD insns.

> 
> >> --- /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.

OK,  I'm tentatively assuming that my changes are well, and I'll talk to lili about putting our patches together to get back to you.
> 
> Jan