FW: [PATCH 3/8] Add tests for APX GPR32 with extend evex prefix

[Cui, Lili]

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: Wednesday, October 18, 2023 4:06 PM
> To: Cui, Lili <lili.cui@intel.com>
> Cc: Lu, Hongjiu <hongjiu.lu@intel.com>; binutils@sourceware.org
> Subject: Re: FW: [PATCH 3/8] Add tests for APX GPR32 with extend evex prefix
> 
> On 18.10.2023 09:16, Cui, Lili wrote:
> >> On 18.10.2023 04:32, Cui, Lili wrote:
> >>>>>>> --- a/gas/testsuite/gas/i386/x86-64-inval-movbe.s
> >>>>>>> +++ b/gas/testsuite/gas/i386/x86-64-inval-movbe.s
> >>>>>>> @@ -1,5 +1,6 @@
> >>>>>>>  # Check illegal movbe in 64bit mode.
> >>>>>>>  	.text
> >>>>>>> +	.arch .noapx_f
> >>>>>>>  foo:
> >>>>>>>  	movbe	(%rcx),%bl
> >>>>>>>  	movbe	%ecx,%ebx
> >>>>>>
> >>>>>> I don't understand the need for this addition (and hence for the
> >>>>>> need to change the test's expecations). Like was mentioned on the
> >>>>>> original
> >>>>>> AVX10 series, tests like this shall not need modification, or
> >>>>>> else it indicates people's code also may need ".arch .noapx_f"
> >>>>>> additions, which I'm sure you agree may not be required. Finally,
> >>>>>> if testcase expecations like the above would be needed anywhere,
> >>>>>> please generalize them such that a similar mere addition of a
> >>>>>> line doesn't require the entire test to be touched. Here this
> >>>>>> means that while for the diagnostics you of course want exact
> >>>>>> line number matches, for the actual listing line numbers don't
> >>>>>> don't need matching
> >> individually.
> >>>>>>
> >>>>>
> >>>>> Agree with you, but movbe is special, movbe didn't support reg to
> >>>>> reg
> >>>> before, but APX enable it. so I added .arch .noapx_f for this invalid test.
> >>>>
> >>>> To be honest I didn't really notice this difference so far. That's
> >>>> somewhat ugly, because people mistakenly using a reg->reg form
> >>>> would suddenly get an EVEX encoding rather than an error. This will
> >>>> need some further thought; please mention the "anomaly" explicitly
> >>>> in the
> >> description of the respective patch.
> >>>>
> >>> SPEC:
> >>> Note: The promoted versions of MOVBE will be extended to include the
> >>> “MOVBE reg1, reg2” form (namely, the ModRM.Mod = 3 case) for both
> >>> opcodes 0xF0 and 0xF1. This extension makes the promotion of BSWAP
> >>> for
> >> NDD support unnecessary.
> >>>
> >>> I'll add a description of movbe to the patch 2/8 changelog.
> >>
> >> Thanks. I did deduce the connection to BSWAP on the way home yesterday.
> >> Which made me come up with a (spec) question: Wouldn't it make sense
> >> to name these
> >> reg->reg forms BSWAP, not MOVBE? It doesn't really matter that they
> >> reg->use the
> >> same encoding as the MOVBE (then mem-only) forms. That would then
> >> eliminate the concern I raised.
> >>
> >
> > If we extend BSWAP, it needs to add the operation of reading ModRM.Mod. I
> think this is the benefit of this solution now.
> 
> I'm afraid I don't follow: I don't propose extending the original BSWAP.
> I merely propose to use the BSWAP mnemonic with the reg->reg form of
> what's presently called MOVBE. (As an aside, in principle no ModR/M byte
> would be needed to extend the original BSWAP: One operand is encoded in
> the base opcode, and the other could be encoded in VVVV. But that would of
> course needlessly consume an entire row in the opcode table. Hence why I
> fully understand that the opcode wants sharing with MOVBE.)
> 

I think it's a trade-off between functionality closer to BSWAP and opcode closer to MOVBE. Since the spec was published and gcc has committed the patch to the community, I think it would be hard to push it to change.

> >> As to 2/8: I received two v2 thereof, with different titles, and a v2 of 3/8.
> >> But nothing else. The list archives match this. I may take a brief
> >> look, but really I'll await a full v2 submission for a proper new review
> round.
> >>
> > After we split a new patch for the EVEX_MAP4_ table, we need to insert a
> patch into the previous patch. I am afraid that if I change the patch number,
> the subsequent review will be easily confused, so I have not changed the patch
> number for the time being.  But for V3 I'll create a new patch  number for it.
> 
> Patch numbering should always match the entire series, plus it should be the
> common case that complete series are sent out, not just new versions of
> individual patches. In the unusual event of nevertheless doing so, patch
> numbering can still be made unambiguous by e.g. (considering this
> case) using 1.9/8, making clear the new patch comes ahead of the revised
> original one. (Whether a new version of a series is shrunk or extended is
> entirely independent of that. E.g. in the case here I could easily see a new
> version combining the two series which were sent initially, for whatever
> reason.)
> 

Got it, thank you very much.

Lili.