[PATCH] x86: Simplify AMX XSAVEC calculation logic.

[Sunil Pandey]

On Thu, Apr 3, 2025 at 9:24 AM Sunil Pandey <skpgkp2@gmail.com> wrote:

>
>
> On Thu, Apr 3, 2025 at 8:55 AM H.J. Lu <hjl.tools@gmail.com> wrote:
>
>> On Wed, Apr 2, 2025 at 7:10 PM H.J. Lu <hjl.tools@gmail.com> wrote:
>> >
>> > On Wed, Apr 2, 2025 at 4:46 PM Sunil Pandey <skpgkp2@gmail.com> wrote:
>> > >
>> > >
>> > >
>> > > On Wed, Apr 2, 2025 at 3:24 PM H.J. Lu <hjl.tools@gmail.com> wrote:
>> > >>
>> > >> On Tue, Apr 1, 2025 at 7:07 AM Sunil Pandey <skpgkp2@gmail.com>
>> wrote:
>> > >> >
>> > >> >
>> > >> >
>> > >> > On Mon, Mar 31, 2025 at 9:25 PM Sunil Pandey <skpgkp2@gmail.com>
>> wrote:
>> > >> >>
>> > >> >>
>> > >> >>
>> > >> >> On Mon, Mar 31, 2025 at 6:13 AM Florian Weimer <
>> fweimer@redhat.com> wrote:
>> > >> >>>
>> > >> >>> * Sunil K. Pandey:
>> > >> >>>
>> > >> >>> > +#ifdef __x86_64__
>> > >> >>> > +                   if (amx_flag)
>> > >> >>> > +                     {
>> > >> >>> > +                       unsigned int amx_size
>> > >> >>> > +                         = xstate_comp_offsets[18] +
>> xstate_comp_sizes[18];
>> > >> >>> > +                       _dl_x86_features_tlsdesc_state_size
>> > >> >>> > +                         = ALIGN_UP (amx_size +
>> TLSDESC_CALL_REGISTER_SAVE_AREA,
>> > >> >>> > +                                     64);
>> > >> >>> > +                     }
>> > >> >>> > +#endif
>> > >> >>>
>> > >> >>> I diskike the magic 18 (as the AMX bit number) appearing here.
>> > >> >>>
>> > >> >>> The logic for including and excluding states seems reversed to
>> me.
>> > >> >>> Shouldn't we save and restore everything that we don't know
>> anything
>> > >> >>> about, especially for the TLSDESC case?
>> > >> >>>
>> > >> >>> Thanks,
>> > >> >>> Florian
>> > >> >>
>> > >> >>
>> > >> >> Thank you so much. Will submit v2.
>> > >> >
>> > >> >
>> > >> > v2 patch attached.
>> > >>
>> > >> Please add more descriptions in the commit log.
>> > >>
>> > >>  #ifdef __x86_64__
>> > >> -       unsigned int amx_size
>> > >> - = (xstate_amx_comp_offsets[31]
>> > >> -    + xstate_amx_comp_sizes[31]);
>> > >> -       amx_size
>> > >> - = ALIGN_UP ((amx_size
>> > >> -      + TLSDESC_CALL_REGISTER_SAVE_AREA),
>> > >> -     64);
>> > >> -       /* Set TLSDESC state size to the compact AMX
>> > >> - state size for XSAVEC.  */
>> > >> -       _dl_x86_features_tlsdesc_state_size = amx_size;
>> > >> +       _dl_x86_features_tlsdesc_state_size
>> > >> + = ALIGN_UP (size + TLSDESC_CALL_REGISTER_SAVE_AREA, 64);
>> > >> +       /* Subtract AMX size from total size.  */
>> > >>
>> > >> Subtract size from the start of X86_XSTATE_TILECFG_ID space to the
>> end of
>> > >> X86_XSTATE_TILEDATA_ID space.
>> > >>
>> > >> +       size -= ((xstate_comp_offsets[18]
>> > >> + + xstate_comp_sizes[18])
>> > >>
>> > >> xstate_comp_offsets[X86_XSTATE_TILEDATA_ID + 1]?
>> > >>
>> > >> +        - (xstate_comp_offsets[16]
>> > >> +   + xstate_comp_sizes[16]));
>> > >>
>> > >> xstate_comp_offsets[X86_XSTATE_TILECFG_ID]?
>> > >
>> > >
>> > >
>> > >  X86_XSTATE_TILEDATA_ID and X86_XSTATE_TILECFG_ID must start from
>> cache aligned location.
>> > > Depending on which previous state gets enabled, size calculation may
>> not be correct.
>> > >
>> > > How about
>> > >
>> > >  size -= (xstate_comp_offsets[X86_XSTATE_TILEDATA_ID + 1]
>> > >               - (xstate_comp_offsets[X86_XSTATE_TILECFG_ID - 1]
>> > >                  + xstate_comp_sizes[X86_XSTATE_TILECFG_ID - 1]));
>> > >
>> >
>> > How about
>> >
>> > size  = ALIGN_UP (size + TLSDESC_CALL_REGISTER_SAVE_AREA,  64);
>> > size -= xstate_comp_offsets[X86_XSTATE_TILEDATA_ID + 1] -
>> > xstate_comp_offsets[X86_XSTATE_TILECFG_ID];
>> >
>> > xstate_comp_offsets[X86_XSTATE_TILECFG_ID] ==
>> > xstate_comp_offsets[X86_XSTATE_TILEDATA_ID + 1]
>> > if there is no AMX.  Otherwise, they are multiples of 64.
>> >
>> >
>> > --
>> > H.J.
>>
>> Please try this.
>>
>>
>> --
>> H.J.
>>
>
> This looks great, as MAX hard coding also gets simplified.
> Testing it out across different platforms.
>
> --Sunil
>

Completed validation on SKL/SKX/SPR/SDE and compared xsave number with
'ld.s. --list-diagnostics" option, no regression.
So this patch looks good to me.

--Sunil
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://sourceware.org/pipermail/libc-alpha/attachments/20250403/31968cfe/attachment.htm>