[PATCH] x86-64: Support APX NF TLS IE with 2 operands

[Jan Beulich]

On 03.07.2024 08:48, H.J. Lu wrote:
> On Wed, Jul 3, 2024, 2:43 PM Jan Beulich <jbeulich@suse.com> wrote:
> 
>> On 03.07.2024 03:55, H.J. Lu wrote:
>>> On Wed, Jul 3, 2024, 9:19 AM kong lingling <lingling.kong7@gmail.com>
>> wrote:
>>>
>>>> Support APX NF TLS IE with 2 operands.Verify it with ld and gold.
>>>>
>>>> gas/
>>>>
>>>>         * config/tc-i386.c (md_assemble): Allow APX NF TLS IE with
>>>>         2 operands.
>>>>         * testsuite/gas/i386/x86-64-gottpoff.d: Updated.
>>>>         * testsuite/gas/i386/x86-64-gottpoff.s: Add APX NF TLS IE
>>>>         tests with 2 operands.
>>>>
>>>> gold/
>>>>
>>>>         * testsuite/x86_64_ie_to_le.s: Add APX NF TLS IE tests with
>>>>         2 operands.
>>>>         * testsuite/x86_64_ie_to_le.sh: Updated.
>>>>
>>>> ld/
>>>>
>>>>         * testsuite/ld-x86-64/tlsbindesc.s: Add APX NF TLS IE tests
>>>>         with 2 operands.
>>>>         * testsuite/ld-x86-64/tlsbindesc.d: Updated.
>>>>         * testsuite/ld-x86-64/tlsbindesc.rd: Likewise.
>>>> ---
>>>>  gas/config/tc-i386.c                     | 10 +++++--
>>>>  gas/testsuite/gas/i386/x86-64-gottpoff.d |  4 +++
>>>>  gas/testsuite/gas/i386/x86-64-gottpoff.s | 10 +++++++
>>>>  gold/testsuite/x86_64_ie_to_le.s         |  1 +
>>>>  gold/testsuite/x86_64_ie_to_le.sh        |  1 +
>>>>  ld/testsuite/ld-x86-64/tlsbindesc.dd     | 12 ++++++++
>>>>  ld/testsuite/ld-x86-64/tlsbindesc.rd     | 36 ++++++++++++------------
>>>>  ld/testsuite/ld-x86-64/tlsbindesc.s      |  4 +++
>>>>  8 files changed, 58 insertions(+), 20 deletions(-)
>>>>
>>>> --- a/gas/config/tc-i386.c
>>>> +++ b/gas/config/tc-i386.c
>>>> @@ -7545,8 +7545,14 @@ md_assemble (char *line)
>>>>                 && i.base_reg
>>>>                 && i.base_reg->reg_num == RegIP
>>>>                 && i.tm.operand_types[0].bitfield.class == Reg
>>>> -               && i.tm.operand_types[2].bitfield.class == Reg)
>>>> -             /* Allow APX: add %reg1, foo@gottpoff(%rip), %reg2.  */
>>>> +               && (i.tm.operand_types[2].bitfield.class == Reg
>>>> +                   || i.tm.operands == 2))
>>>> +             /* Allow APX:
>>>> +                add %reg1, foo@gottpoff(%rip), %reg2
>>>> +                add foo@gottpoff(%rip), %reg, %reg2
>>>> +                {nf} add foo@gottpoff(%rip), %reg
>>>> +                {nf} add %reg1, foo@gottpoff(%rip), %reg2
>>>> +                {nf} add foo@gottpoff(%rip), %reg, %reg2.  */
>>>>               break;
>>>>             /* Fall through.  */
>>>>           case BFD_RELOC_386_TLS_GOTIE:
>>>> [...]
>>>> --
>>>> 2.31.1
>>>>
>>>
>>> OK.
>>
>> H.J., please don't do this when you're well aware that earlier comments
>> of others (me in this case) were not addressed. The code left in context
>> above _STILL_ permits memory destinations for the 2-operand case, despite
>> the comment saying otherwise. Plus the EVEX and legacy cases are still
>> being treated vastly different.
>>
>> Lingling, I notice you committed the patch with H.J.'s approval above,
>> despite knowing there were open issues. I'm going to expect an
>> incremental change to at least address the former of the two issues.
>> Should that not arrive within a couple of days, I'm afraid I'm going to
>> need to revert your change, for having been committed prematurely /
>> unduly. You having done so is even more so odd because the requested
>> adjustment would actually have simplified the code:
>>
>>             if (i.tm.mnem_off == MN_add
>>                 && i.tm.opcode_space == SPACE_EVEXMAP4
>>                 && i.mem_operands == 1
>>                 && i.base_reg
>>                 && i.base_reg->reg_num == RegIP
>>                 && i.tm.operand_types[0].bitfield.class == Reg
>>                 && i.tm.operand_types[i.operands - 1].bitfield.class ==
>> Reg)
>>
>> For the latter of the two issues, if H.J. is unwilling to actually
>> settle on consistent criteria between legacy and EVEX encodings, I guess
>> it'll end up being me to actually make this code consistent, one way or
>> another. The unwillingness to settle on criteria up front means that
>> there then shall not be objections later on.
> 
> Please open a bug report with a testcase.

You're kidding? The inconsistency is blatantly obvious. And any testcase is
going to be contrived anyway, as what we're discussing here is inconsistent
application of an underlying, unwritten policy: How much is the assembler
supposed to be refusing? How much control is the assembler supposed to be
leaving to the programmer? I can live with such a policy being unwritten; I
can't accept such a policy to be applied inconsistently (and hence
unpredictably for the programmer).

We did discuss all the same underlying issue with how you're approaching
such things already when / after you introduced the entirely arbitrary KMOV
special case (see your commit d7e3e627027f vs my later e3669c7f7ba4).

Jan