Re: [PATCH v4 0/9] Support Intel APX EGPR

[Jan Beulich <jbeulich@suse.com>]

On 20.12.2023 12:50, Cui, Lili wrote:
>>>>>> On 19.12.2023 13:12, Cui, Lili wrote:
>>>>>>> *** BLURB HERE ***
>>>>>>> Future optimizations to be made.
>>>>>>> 1. The current implementation of vexvvvvv needs to be optimized.
>>>>>>> 2. The handling of double VEX/EVEX templates in check_register()
>>>>>>> needs to
>>>>>> be optimized.
>>>>>>
>>>>>> I hope this is just stale here, and the dependency on templates was
>>>>>> now removed again from check_register().
>>>>>
>>>>> In fact, I didn't remove it in V4, I didn't find a better place to
>>>>> deal with it. I
>>>> don't know if you agree with this implementation below.
>>>>
>>>> I'm afraid I don't, both because it still isn't clear to me what's
>>>> wrong with my alternative proposal, and also for the formal reason of ...
>>>>
>>>
>>> For the alternative proposal, do you mean adding a new variable to avoid
>> introducing new loops over all operands? How about this ? or do you want to
>> add other variable and handle it in check_register?
>>
>> No, the alternative proposal continues to be to introduce a new enumerator
>> to record in i.vec_encoding (vex_encoding_egpr is what iirc I had suggested
>> before, despite the naming anomaly). What you outline below would,
>> however, still be better than adding another loop (as you had it earlier), imo.
>>
> 
> I guessed you want to add a new type like vex_encoding_egpr, but I don't know how to do it differently with before, when the instruction support legacy, vex and evex encodings, if we put the vex and eves templates in front of the legacy templates (in i386-opc.tbl), we'll assign the vex_encoding_egpr for the legacy input, and it will have the same problem as before. And we also need to handle it in check_register(). Maybe you hinted at some other way of handling it, but I didn't get it.
> 
> 
>      if (current_templates.start->opcode_modifier.vex
>         && current_templates.start->opcode_modifier.evex)
>       i.vec_encoding = vex_encoding_egpr;

Since setting of the new encoding type has to happen in check_register(),
using current_templates (as said several times before) is not an option.

Anyway, in the interest of forward progress, feel free to go with ...

>>> --- a/gas/config/tc-i386.c
>>> +++ b/gas/config/tc-i386.c
>>> @@ -464,6 +464,9 @@ struct _i386_insn
>>>      /* Have NOTRACK prefix.  */
>>>      const char *notrack_prefix;
>>>
>>> +    /* Has Egpr.  */
>>> +    bool has_egpr;
>>> +
>>>      /* Error message.  */
>>>      enum i386_error error;
>>>    };
>>
>> As a general remark, when you add new fields to a struct, please try to find a
>> slot that ideally is using existing padding _and_ is next to related fields, or at
>> least one of the two.
>>
> 
> Moved to
> 
> --- a/gas/config/tc-i386.c
> +++ b/gas/config/tc-i386.c
> @@ -438,6 +438,9 @@ struct _i386_insn
>      /* Prefer the REX2 prefix in encoding.  */
>      bool rex2_encoding;
> 
> +    /* Has Egpr.  */
> +    bool has_egpr;

... this approach then, and subsequently I'll see if I can re-arrange things
(and if I'm bothered enough to do so). The comment is pretty unhelpful as is,
how about "Need to use an eGPR capable encoding (REX2 or EVEX)" or some such?

Jan