[PATCH] x86: Simplify AMX XSAVEC calculation logic.

[Sunil Pandey]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://sourceware.org/pipermail/libc-alpha/attachments/20250403/b146150f/attachment.htm>