[PATCH] x86: Simplify AMX XSAVEC calculation logic.

[Sunil Pandey]

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]));



> --
> H.J.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://sourceware.org/pipermail/libc-alpha/attachments/20250402/855eb35f/attachment-0001.htm>