Re: [PATCH 2/2] i386: correct x87&SSE multiplication modeling in znver.md

["Jan Hubička" <honza.hubicka@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2762 bytes --]

On Wed, Nov 16, 2022 at 2:13 PM Alexander Monakov <amonakov@ispras.ru>
wrote:

>
> On Wed, 16 Nov 2022, Jan Hubička wrote:
>
> > This looks really promising.  I will experiment with the patch for
> separate
> > znver3 model, but I think we should be able to keep
> > them unified and hopefully get both less code duplicatoin and table
> sizes.
>
> Do you mean separate znver4 (not '3') model (i.e. the recent patch by AMD)?
>

Yes. I guess we want to check what variant leads to smaller automaton.  I
would somewhat prefer to keep the models unified since they are quite
similar

> > > znver1 native path is 128  and znver2/3 has 256 bit paths.
> > > We need to split this into two reservations. One for znver1 and the
> other
> > > for znver2/3.
> > >
> >
> > isn't it znver2 for 128 and znver3 for 256?
>
> No, Zen 1 splits AVX256 instructions into pairs of 128-bit uops, Zen 2 and
> Zen 3 have native 256-bit units. Zen 4 again executes AVX512 instructions
> on 256-bit units.
>

Ah, of course.  I mixed things up in my memory. Sorry fro that.

>
> I think a split is not needed because the preceding reservation already
> handles
> znver2 and znver3, we just need to remove them here, like this:
>
> diff --git a/gcc/config/i386/znver.md b/gcc/config/i386/znver.md
> index 882f250f1..16b5afa5d 100644
> --- a/gcc/config/i386/znver.md
> +++ b/gcc/config/i386/znver.md
> @@ -1242,7 +1242,7 @@ (define_insn_reservation "znver1_sseimul" 3
>                          "znver1-direct,znver1-fp0")
>
>  (define_insn_reservation "znver1_sseimul_avx256" 4
> -                        (and (eq_attr "cpu" "znver1,znver2,znver3")
> +                        (and (eq_attr "cpu" "znver1")
>

It should work even without removal since first reservation matches, but
this is quite less confusing indeed.

>                               (and (eq_attr "mode" "OI")
>                                    (and (eq_attr "type" "sseimul")
>                                         (eq_attr "memory" "none"))))
> @@ -1260,7 +1260,7 @@ (define_insn_reservation "znver1_sseimul_load" 10
>                          "znver1-direct,znver1-load,znver1-fp0")
>
>  (define_insn_reservation "znver1_sseimul_avx256_load" 11
> -                        (and (eq_attr "cpu" "znver1,znver2,znver3")
> +                        (and (eq_attr "cpu" "znver1")
>                               (and (eq_attr "mode" "OI")
>                                    (and (eq_attr "type" "sseimul")
>                                         (eq_attr "memory" "load"))))
>
> > The patch looks good.
> > >
> > Patch is OK then :)
>
> For *both* patches in the series?
>

Yes, thanks a lot for looking into this!
Honza

>
> Alexander