[v3] x86-64: Add Avoid_Short_Distance_REP_MOVSB
Checks
| Context |
Check |
Description |
| dj/TryBot-apply_patch |
success
|
Patch applied to master at the time it was sent
|
| dj/TryBot-32bit |
fail
|
Patch series failed to apply
|
Commit Message
On Tue, Jul 27, 2021 at 12:12 PM Noah Goldstein <goldstein.w.n@gmail.com> wrote:
>
>
>
> On Tue, Jul 27, 2021 at 12:06 PM H.J. Lu <hjl.tools@gmail.com> wrote:
>>
>> On Mon, Jul 26, 2021 at 9:06 PM Noah Goldstein <goldstein.w.n@gmail.com> wrote:
>> >
>> >
>> >
>> > On Mon, Jul 26, 2021 at 11:11 PM H.J. Lu via Libc-alpha <libc-alpha@sourceware.org> wrote:
>> >>
>> >> On Mon, Jul 26, 2021 at 7:15 PM Carlos O'Donell <carlos@redhat.com> wrote:
>> >> >
>> >> > On 7/26/21 8:00 AM, H.J. Lu via Libc-alpha wrote:
>> >> > > commit 3ec5d83d2a237d39e7fd6ef7a0bc8ac4c171a4a5
>> >> > > Author: H.J. Lu <hjl.tools@gmail.com>
>> >> > > Date: Sat Jan 25 14:19:40 2020 -0800
>> >> > >
>> >> > > x86-64: Avoid rep movsb with short distance [BZ #27130]
>> >> > > introduced some regressions on Intel processors without Fast Short REP
>> >> > > MOV (FSRM). Add Avoid_Short_Distance_REP_MOVSB to avoid rep movsb with
>> >> > > short distance only on Intel processors with FSRM. bench-memmove-large
>> >> > > on Skylake server shows that cycles of __memmove_evex_unaligned_erms are
>> >> > > improved for the following data size:
>> >> > >
>> >> > > before after Improvement
>> >> > > length=4127, align1=3, align2=0: 479.38 343.00 28%
>> >> > > length=4223, align1=9, align2=5: 405.62 335.50 17%
>> >> > > length=8223, align1=3, align2=0: 786.12 495.00 37%
>> >> > > length=8319, align1=9, align2=5: 256.69 170.38 33%
>> >> > > length=16415, align1=3, align2=0: 1436.88 839.50 41%
>> >> > > length=16511, align1=9, align2=5: 1375.50 840.62 39%
>> >> > > length=32799, align1=3, align2=0: 2890.00 1850.62 36%
>> >> > > length=32895, align1=9, align2=5: 2891.38 1948.62 32%
>> >> > >
>> >> > > There are no regression on Ice Lake server.
>> >> >
>> >> > At this point we're waiting on Noah to provide feedback on the performance
>> >> > results given the alignment nop insertion you provided as a follow-up patch
>> >
>> >
>> > The results with the padding look good!
>> >
>> >>
>> >>
>> >> We are testing 25 byte nop padding now:
>> >>
>> >>
>> >> https://gitlab.com/x86-glibc/glibc/-/commit/de8985640a568786a59576716db54e0749d420e8
>> >>
>> > How did you come to the exact padding choice used?
>>
>> I first replaced the 9 byte instructions:
>>
>> andl $X86_STRING_CONTROL_AVOID_SHORT_DISTANCE_REP_MOVSB,
>> __x86_string_control(%rip)
>> jz 3f
>>
>> with a 9-byte NOP and reproduced the regression on Tiger Lake. It confirmed
>> that the code layout caused the regression. I first tried adding
>> ".p2align 4" to
>> branch targets and they made no differences. Then I started adding different
>> size of nops after
>>
>> andl $X86_STRING_CONTROL_AVOID_SHORT_DISTANCE_REP_MOVSB,
>> __x86_string_control(%rip)
>> jz 3f
>> movq %rdi, %rcx
>> subq %rsi, %rcx
>> jmp 2f
>>
>> with ".nops N". I started with N == 1 and doubled N in each step. I
>> noticed that
>> improvement started at N == 32. I started bisecting between 16 and 32:
>>
>> 1. 24 and 32 are good.
>> 2. 24 and 28 are good.
>> 3. 25 is the best overall.
>>
>> >>
>> >> > (unless you can confirm this yourself).
>> >> >
>> >> > Looking forward to a v2 the incorporates the alignment fix (pending Noah's
>> >> > comments), and my suggestions below.
>> >>
>> >> >
>> >> > > ---
>> >> > > sysdeps/x86/cacheinfo.h | 7 +++++++
>> >> > > sysdeps/x86/cpu-features.c | 5 +++++
>> >> > > .../x86/include/cpu-features-preferred_feature_index_1.def | 1 +
>> >> > > sysdeps/x86/sysdep.h | 3 +++
>> >> > > sysdeps/x86_64/multiarch/memmove-vec-unaligned-erms.S | 5 +++++
>> >> > > 5 files changed, 21 insertions(+)
>> >> > >
>> >> > > diff --git a/sysdeps/x86/cacheinfo.h b/sysdeps/x86/cacheinfo.h
>> >> > > index eba8dbc4a6..174ea38f5b 100644
>> >> > > --- a/sysdeps/x86/cacheinfo.h
>> >> > > +++ b/sysdeps/x86/cacheinfo.h
>> >> > > @@ -49,6 +49,9 @@ long int __x86_rep_stosb_threshold attribute_hidden = 2048;
>> >> > > /* Threshold to stop using Enhanced REP MOVSB. */
>> >> > > long int __x86_rep_movsb_stop_threshold attribute_hidden;
>> >> > >
>> >> > > +/* String/memory function control. */
>> >> > > +int __x86_string_control attribute_hidden;
>> >> >
>> >> > Please expand comment.
>> >> >
>> >> > Suggest:
>> >> >
>> >> > /* A bit-wise OR of string/memory requirements for optimal performance
>> >> > e.g. X86_STRING_CONTROL_AVOID_SHORT_DISTANCE_REP_MOVSB. These bits
>> >> > are used at runtime to tune implementation behavior. */
>> >> > int __x86_string_control attribute_hidden;
>> >>
>> >> I will fix it in the v2 patch.
>> >>
>> >> Thanks.
>> >>
>> >> > > +
>> >> > > static void
>> >> > > init_cacheinfo (void)
>> >> > > {
>> >> > > @@ -71,5 +74,9 @@ init_cacheinfo (void)
>> >> > > __x86_rep_movsb_threshold = cpu_features->rep_movsb_threshold;
>> >> > > __x86_rep_stosb_threshold = cpu_features->rep_stosb_threshold;
>> >> > > __x86_rep_movsb_stop_threshold = cpu_features->rep_movsb_stop_threshold;
>> >> > > +
>> >> > > + if (CPU_FEATURES_ARCH_P (cpu_features, Avoid_Short_Distance_REP_MOVSB))
>> >> > > + __x86_string_control
>> >> > > + |= X86_STRING_CONTROL_AVOID_SHORT_DISTANCE_REP_MOVSB;
>> >> >
>> >> > OK.
>> >> >
>> >> > > }
>> >> > > #endif
>> >> > > diff --git a/sysdeps/x86/cpu-features.c b/sysdeps/x86/cpu-features.c
>> >> > > index 706a172ba9..645bba6314 100644
>> >> > > --- a/sysdeps/x86/cpu-features.c
>> >> > > +++ b/sysdeps/x86/cpu-features.c
>> >> > > @@ -555,6 +555,11 @@ init_cpu_features (struct cpu_features *cpu_features)
>> >> > > cpu_features->preferred[index_arch_Prefer_AVX2_STRCMP]
>> >> > > |= bit_arch_Prefer_AVX2_STRCMP;
>> >> > > }
>> >> > > +
>> >> > > + /* Avoid avoid short distance REP MOVSB on processor with FSRM. */
>> >> > > + if (CPU_FEATURES_CPU_P (cpu_features, FSRM))
>> >> > > + cpu_features->preferred[index_arch_Avoid_Short_Distance_REP_MOVSB]
>> >> > > + |= bit_arch_Avoid_Short_Distance_REP_MOVSB;
>> >> >
>> >> > OK.
>> >> >
>> >> > > }
>> >> > > /* This spells out "AuthenticAMD" or "HygonGenuine". */
>> >> > > else if ((ebx == 0x68747541 && ecx == 0x444d4163 && edx == 0x69746e65)
>> >> > > diff --git a/sysdeps/x86/include/cpu-features-preferred_feature_index_1.def b/sysdeps/x86/include/cpu-features-preferred_feature_index_1.def
>> >> > > index 133aab19f1..d7c93f00c5 100644
>> >> > > --- a/sysdeps/x86/include/cpu-features-preferred_feature_index_1.def
>> >> > > +++ b/sysdeps/x86/include/cpu-features-preferred_feature_index_1.def
>> >> > > @@ -33,3 +33,4 @@ BIT (Prefer_No_AVX512)
>> >> > > BIT (MathVec_Prefer_No_AVX512)
>> >> > > BIT (Prefer_FSRM)
>> >> > > BIT (Prefer_AVX2_STRCMP)
>> >> > > +BIT (Avoid_Short_Distance_REP_MOVSB)
>> >> >
>> >> > OK.
>> >> >
>> >> > > diff --git a/sysdeps/x86/sysdep.h b/sysdeps/x86/sysdep.h
>> >> > > index 51c069bfe1..35cb90d507 100644
>> >> > > --- a/sysdeps/x86/sysdep.h
>> >> > > +++ b/sysdeps/x86/sysdep.h
>> >> > > @@ -57,6 +57,9 @@ enum cf_protection_level
>> >> > > #define STATE_SAVE_MASK \
>> >> > > ((1 << 1) | (1 << 2) | (1 << 3) | (1 << 5) | (1 << 6) | (1 << 7))
>> >> > >
>> >> >
>> >> > Suggest adding:
>> >> >
>> >> > /* Constants for bits in __x86_string_control: */
>> >> >
>> >> > > +/* Avoid short distance REP MOVSB. */
>> >> > > +#define X86_STRING_CONTROL_AVOID_SHORT_DISTANCE_REP_MOVSB (1 << 0)
>> >> >
>> >> > OK.
>> >> >
>> >> > > +
>> >> > > #ifdef __ASSEMBLER__
>> >> > >
>> >> > > /* Syntactic details of assembler. */
>> >> > > diff --git a/sysdeps/x86_64/multiarch/memmove-vec-unaligned-erms.S b/sysdeps/x86_64/multiarch/memmove-vec-unaligned-erms.S
>> >> > > index a783da5de2..9f02624375 100644
>> >> > > --- a/sysdeps/x86_64/multiarch/memmove-vec-unaligned-erms.S
>> >> > > +++ b/sysdeps/x86_64/multiarch/memmove-vec-unaligned-erms.S
>> >> > > @@ -325,12 +325,16 @@ L(movsb):
>> >> > > /* Avoid slow backward REP MOVSB. */
>> >> > > jb L(more_8x_vec_backward)
>> >> > > # if AVOID_SHORT_DISTANCE_REP_MOVSB
>> >> > > + andl $X86_STRING_CONTROL_AVOID_SHORT_DISTANCE_REP_MOVSB, __x86_string_control(%rip)
>> >> > > + jz 3f
>> >> >
>> >> > OK.
>> >> >
>> >> > > movq %rdi, %rcx
>> >> > > subq %rsi, %rcx
>> >> > > jmp 2f
>> >> > > # endif
>> >> > > 1:
>> >> > > # if AVOID_SHORT_DISTANCE_REP_MOVSB
>> >> > > + andl $X86_STRING_CONTROL_AVOID_SHORT_DISTANCE_REP_MOVSB, __x86_string_control(%rip)
>> >> > > + jz 3f
>> >> >
>> >> > OK.
>> >> >
>> >> > > movq %rsi, %rcx
>> >> > > subq %rdi, %rcx
>> >> > > 2:
>> >> > > @@ -338,6 +342,7 @@ L(movsb):
>> >> > > is N*4GB + [1..63] with N >= 0. */
>> >> > > cmpl $63, %ecx
>> >> > > jbe L(more_2x_vec) /* Avoid "rep movsb" if ECX <= 63. */
>> >> > > +3:
>> >> >
>> >> > OK.
>> >> >
>> >> > > # endif
>> >> > > mov %RDX_LP, %RCX_LP
>> >> > > rep movsb
>> >> > >
>> >> >
>> >> >
>> >> > --
>> >> > Cheers,
>> >> > Carlos.
>> >> >
>> >>
>> >>
>> >> --
>> >> H.J.
>>
>> Here is the v2 patch:
>>
>> 1. Add a 25-byte NOP padding after JMP for Avoid_Short_Distance_REP_MOVSB,
>> which improves bench-memcpy-random performance on Tiger Lake by ~30%
>
>
> I think this may not be due to unrelated factors. I reran the random benchmarks with
> the function on a fresh page and entry of the *_erms version at either + 0, 16, 32, 48
> bytes and 1) don't see a universal improvement and 2) believe it's likely that the 30%
> you measured is due to unrelated alignment changes.
>
> "__memcpy_avx_unaligned", "__memcpy_avx_unaligned_erms", "__memcpy_evex_unaligned", "__memcpy_evex_unaligned_erms"
>
> + 0 Entry Alignment
> New: 91824.1, 95460.1, 95063.3, 97998.3
> Old: 99973.7, 100127, 100370, 100049
>
> + 16 Entry Alignment
> New: 129558, 129916, 122373, 124056
> Old: 125361, 96475.4, 97457.8, 124319
>
> + 32 Entry Alignment
> New: 95073.7, 92857.8, 90182.2, 92666.3
> Old: 96702.1, 98558.9, 96797.1, 96887.1
>
> + 48 Entry Alignment
> New: 135161, 134010, 123023, 148589
> Old: 128150, 139029, 98382.2, 122686
>
> So the 32 byte/64 byte entry alignment versions seem to favor this change,
> but when entry alignment % 16 != 0 this change seems to perform worse.
>
> I generally think until we understand why the byte padding is necessary its
> probably a mistake to include it unless its a benefit in actual SPEC2017.
>
> My general intuition is that this is an issue with the benchmarks themselves
> so I support the change w.o the padding.
Here is the v3 patch without the padding. OK for master?
>
>> 2. Update comments for __x86_string_control.
>>
>>
>> --
>> H.J.
Comments
On Tue, Jul 27, 2021 at 3:23 PM H.J. Lu <hjl.tools@gmail.com> wrote:
> On Tue, Jul 27, 2021 at 12:12 PM Noah Goldstein <goldstein.w.n@gmail.com>
> wrote:
> >
> >
> >
> > On Tue, Jul 27, 2021 at 12:06 PM H.J. Lu <hjl.tools@gmail.com> wrote:
> >>
> >> On Mon, Jul 26, 2021 at 9:06 PM Noah Goldstein <goldstein.w.n@gmail.com>
> wrote:
> >> >
> >> >
> >> >
> >> > On Mon, Jul 26, 2021 at 11:11 PM H.J. Lu via Libc-alpha <
> libc-alpha@sourceware.org> wrote:
> >> >>
> >> >> On Mon, Jul 26, 2021 at 7:15 PM Carlos O'Donell <carlos@redhat.com>
> wrote:
> >> >> >
> >> >> > On 7/26/21 8:00 AM, H.J. Lu via Libc-alpha wrote:
> >> >> > > commit 3ec5d83d2a237d39e7fd6ef7a0bc8ac4c171a4a5
> >> >> > > Author: H.J. Lu <hjl.tools@gmail.com>
> >> >> > > Date: Sat Jan 25 14:19:40 2020 -0800
> >> >> > >
> >> >> > > x86-64: Avoid rep movsb with short distance [BZ #27130]
> >> >> > > introduced some regressions on Intel processors without Fast
> Short REP
> >> >> > > MOV (FSRM). Add Avoid_Short_Distance_REP_MOVSB to avoid rep
> movsb with
> >> >> > > short distance only on Intel processors with FSRM.
> bench-memmove-large
> >> >> > > on Skylake server shows that cycles of
> __memmove_evex_unaligned_erms are
> >> >> > > improved for the following data size:
> >> >> > >
> >> >> > > before after Improvement
> >> >> > > length=4127, align1=3, align2=0: 479.38 343.00 28%
> >> >> > > length=4223, align1=9, align2=5: 405.62 335.50 17%
> >> >> > > length=8223, align1=3, align2=0: 786.12 495.00 37%
> >> >> > > length=8319, align1=9, align2=5: 256.69 170.38 33%
> >> >> > > length=16415, align1=3, align2=0: 1436.88 839.50 41%
> >> >> > > length=16511, align1=9, align2=5: 1375.50 840.62 39%
> >> >> > > length=32799, align1=3, align2=0: 2890.00 1850.62 36%
> >> >> > > length=32895, align1=9, align2=5: 2891.38 1948.62 32%
> >> >> > >
> >> >> > > There are no regression on Ice Lake server.
> >> >> >
> >> >> > At this point we're waiting on Noah to provide feedback on the
> performance
> >> >> > results given the alignment nop insertion you provided as a
> follow-up patch
> >> >
> >> >
> >> > The results with the padding look good!
> >> >
> >> >>
> >> >>
> >> >> We are testing 25 byte nop padding now:
> >> >>
> >> >>
> >> >>
> https://gitlab.com/x86-glibc/glibc/-/commit/de8985640a568786a59576716db54e0749d420e8
> >> >>
> >> > How did you come to the exact padding choice used?
> >>
> >> I first replaced the 9 byte instructions:
> >>
> >> andl $X86_STRING_CONTROL_AVOID_SHORT_DISTANCE_REP_MOVSB,
> >> __x86_string_control(%rip)
> >> jz 3f
> >>
> >> with a 9-byte NOP and reproduced the regression on Tiger Lake. It
> confirmed
> >> that the code layout caused the regression. I first tried adding
> >> ".p2align 4" to
> >> branch targets and they made no differences. Then I started adding
> different
> >> size of nops after
> >>
> >> andl $X86_STRING_CONTROL_AVOID_SHORT_DISTANCE_REP_MOVSB,
> >> __x86_string_control(%rip)
> >> jz 3f
> >> movq %rdi, %rcx
> >> subq %rsi, %rcx
> >> jmp 2f
> >>
> >> with ".nops N". I started with N == 1 and doubled N in each step. I
> >> noticed that
> >> improvement started at N == 32. I started bisecting between 16 and 32:
> >>
> >> 1. 24 and 32 are good.
> >> 2. 24 and 28 are good.
> >> 3. 25 is the best overall.
> >>
> >> >>
> >> >> > (unless you can confirm this yourself).
> >> >> >
> >> >> > Looking forward to a v2 the incorporates the alignment fix
> (pending Noah's
> >> >> > comments), and my suggestions below.
> >> >>
> >> >> >
> >> >> > > ---
> >> >> > > sysdeps/x86/cacheinfo.h | 7
> +++++++
> >> >> > > sysdeps/x86/cpu-features.c | 5
> +++++
> >> >> > > .../x86/include/cpu-features-preferred_feature_index_1.def | 1 +
> >> >> > > sysdeps/x86/sysdep.h | 3
> +++
> >> >> > > sysdeps/x86_64/multiarch/memmove-vec-unaligned-erms.S | 5
> +++++
> >> >> > > 5 files changed, 21 insertions(+)
> >> >> > >
> >> >> > > diff --git a/sysdeps/x86/cacheinfo.h b/sysdeps/x86/cacheinfo.h
> >> >> > > index eba8dbc4a6..174ea38f5b 100644
> >> >> > > --- a/sysdeps/x86/cacheinfo.h
> >> >> > > +++ b/sysdeps/x86/cacheinfo.h
> >> >> > > @@ -49,6 +49,9 @@ long int __x86_rep_stosb_threshold
> attribute_hidden = 2048;
> >> >> > > /* Threshold to stop using Enhanced REP MOVSB. */
> >> >> > > long int __x86_rep_movsb_stop_threshold attribute_hidden;
> >> >> > >
> >> >> > > +/* String/memory function control. */
> >> >> > > +int __x86_string_control attribute_hidden;
> >> >> >
> >> >> > Please expand comment.
> >> >> >
> >> >> > Suggest:
> >> >> >
> >> >> > /* A bit-wise OR of string/memory requirements for optimal
> performance
> >> >> > e.g. X86_STRING_CONTROL_AVOID_SHORT_DISTANCE_REP_MOVSB. These
> bits
> >> >> > are used at runtime to tune implementation behavior. */
> >> >> > int __x86_string_control attribute_hidden;
> >> >>
> >> >> I will fix it in the v2 patch.
> >> >>
> >> >> Thanks.
> >> >>
> >> >> > > +
> >> >> > > static void
> >> >> > > init_cacheinfo (void)
> >> >> > > {
> >> >> > > @@ -71,5 +74,9 @@ init_cacheinfo (void)
> >> >> > > __x86_rep_movsb_threshold = cpu_features->rep_movsb_threshold;
> >> >> > > __x86_rep_stosb_threshold = cpu_features->rep_stosb_threshold;
> >> >> > > __x86_rep_movsb_stop_threshold =
> cpu_features->rep_movsb_stop_threshold;
> >> >> > > +
> >> >> > > + if (CPU_FEATURES_ARCH_P (cpu_features,
> Avoid_Short_Distance_REP_MOVSB))
> >> >> > > + __x86_string_control
> >> >> > > + |= X86_STRING_CONTROL_AVOID_SHORT_DISTANCE_REP_MOVSB;
> >> >> >
> >> >> > OK.
> >> >> >
> >> >> > > }
> >> >> > > #endif
> >> >> > > diff --git a/sysdeps/x86/cpu-features.c
> b/sysdeps/x86/cpu-features.c
> >> >> > > index 706a172ba9..645bba6314 100644
> >> >> > > --- a/sysdeps/x86/cpu-features.c
> >> >> > > +++ b/sysdeps/x86/cpu-features.c
> >> >> > > @@ -555,6 +555,11 @@ init_cpu_features (struct cpu_features
> *cpu_features)
> >> >> > > cpu_features->preferred[index_arch_Prefer_AVX2_STRCMP]
> >> >> > > |= bit_arch_Prefer_AVX2_STRCMP;
> >> >> > > }
> >> >> > > +
> >> >> > > + /* Avoid avoid short distance REP MOVSB on processor with
> FSRM. */
> >> >> > > + if (CPU_FEATURES_CPU_P (cpu_features, FSRM))
> >> >> > > +
> cpu_features->preferred[index_arch_Avoid_Short_Distance_REP_MOVSB]
> >> >> > > + |= bit_arch_Avoid_Short_Distance_REP_MOVSB;
> >> >> >
> >> >> > OK.
> >> >> >
> >> >> > > }
> >> >> > > /* This spells out "AuthenticAMD" or "HygonGenuine". */
> >> >> > > else if ((ebx == 0x68747541 && ecx == 0x444d4163 && edx ==
> 0x69746e65)
> >> >> > > diff --git
> a/sysdeps/x86/include/cpu-features-preferred_feature_index_1.def
> b/sysdeps/x86/include/cpu-features-preferred_feature_index_1.def
> >> >> > > index 133aab19f1..d7c93f00c5 100644
> >> >> > > ---
> a/sysdeps/x86/include/cpu-features-preferred_feature_index_1.def
> >> >> > > +++
> b/sysdeps/x86/include/cpu-features-preferred_feature_index_1.def
> >> >> > > @@ -33,3 +33,4 @@ BIT (Prefer_No_AVX512)
> >> >> > > BIT (MathVec_Prefer_No_AVX512)
> >> >> > > BIT (Prefer_FSRM)
> >> >> > > BIT (Prefer_AVX2_STRCMP)
> >> >> > > +BIT (Avoid_Short_Distance_REP_MOVSB)
> >> >> >
> >> >> > OK.
> >> >> >
> >> >> > > diff --git a/sysdeps/x86/sysdep.h b/sysdeps/x86/sysdep.h
> >> >> > > index 51c069bfe1..35cb90d507 100644
> >> >> > > --- a/sysdeps/x86/sysdep.h
> >> >> > > +++ b/sysdeps/x86/sysdep.h
> >> >> > > @@ -57,6 +57,9 @@ enum cf_protection_level
> >> >> > > #define STATE_SAVE_MASK \
> >> >> > > ((1 << 1) | (1 << 2) | (1 << 3) | (1 << 5) | (1 << 6) | (1 <<
> 7))
> >> >> > >
> >> >> >
> >> >> > Suggest adding:
> >> >> >
> >> >> > /* Constants for bits in __x86_string_control: */
> >> >> >
> >> >> > > +/* Avoid short distance REP MOVSB. */
> >> >> > > +#define X86_STRING_CONTROL_AVOID_SHORT_DISTANCE_REP_MOVSB (1
> << 0)
> >> >> >
> >> >> > OK.
> >> >> >
> >> >> > > +
> >> >> > > #ifdef __ASSEMBLER__
> >> >> > >
> >> >> > > /* Syntactic details of assembler. */
> >> >> > > diff --git
> a/sysdeps/x86_64/multiarch/memmove-vec-unaligned-erms.S
> b/sysdeps/x86_64/multiarch/memmove-vec-unaligned-erms.S
> >> >> > > index a783da5de2..9f02624375 100644
> >> >> > > --- a/sysdeps/x86_64/multiarch/memmove-vec-unaligned-erms.S
> >> >> > > +++ b/sysdeps/x86_64/multiarch/memmove-vec-unaligned-erms.S
> >> >> > > @@ -325,12 +325,16 @@ L(movsb):
> >> >> > > /* Avoid slow backward REP MOVSB. */
> >> >> > > jb L(more_8x_vec_backward)
> >> >> > > # if AVOID_SHORT_DISTANCE_REP_MOVSB
> >> >> > > + andl
> $X86_STRING_CONTROL_AVOID_SHORT_DISTANCE_REP_MOVSB,
> __x86_string_control(%rip)
> >> >> > > + jz 3f
> >> >> >
> >> >> > OK.
> >> >> >
> >> >> > > movq %rdi, %rcx
> >> >> > > subq %rsi, %rcx
> >> >> > > jmp 2f
> >> >> > > # endif
> >> >> > > 1:
> >> >> > > # if AVOID_SHORT_DISTANCE_REP_MOVSB
> >> >> > > + andl
> $X86_STRING_CONTROL_AVOID_SHORT_DISTANCE_REP_MOVSB,
> __x86_string_control(%rip)
> >> >> > > + jz 3f
> >> >> >
> >> >> > OK.
> >> >> >
> >> >> > > movq %rsi, %rcx
> >> >> > > subq %rdi, %rcx
> >> >> > > 2:
> >> >> > > @@ -338,6 +342,7 @@ L(movsb):
> >> >> > > is N*4GB + [1..63] with N >= 0. */
> >> >> > > cmpl $63, %ecx
> >> >> > > jbe L(more_2x_vec) /* Avoid "rep movsb" if ECX <=
> 63. */
> >> >> > > +3:
> >> >> >
> >> >> > OK.
> >> >> >
> >> >> > > # endif
> >> >> > > mov %RDX_LP, %RCX_LP
> >> >> > > rep movsb
> >> >> > >
> >> >> >
> >> >> >
> >> >> > --
> >> >> > Cheers,
> >> >> > Carlos.
> >> >> >
> >> >>
> >> >>
> >> >> --
> >> >> H.J.
> >>
> >> Here is the v2 patch:
> >>
> >> 1. Add a 25-byte NOP padding after JMP for
> Avoid_Short_Distance_REP_MOVSB,
> >> which improves bench-memcpy-random performance on Tiger Lake by ~30%
> >
> >
> > I think this may not be due to unrelated factors. I reran the random
> benchmarks with
> > the function on a fresh page and entry of the *_erms version at either +
> 0, 16, 32, 48
> > bytes and 1) don't see a universal improvement and 2) believe it's
> likely that the 30%
> > you measured is due to unrelated alignment changes.
> >
> > "__memcpy_avx_unaligned", "__memcpy_avx_unaligned_erms",
> "__memcpy_evex_unaligned", "__memcpy_evex_unaligned_erms"
> >
> > + 0 Entry Alignment
> > New: 91824.1, 95460.1, 95063.3, 97998.3
> > Old: 99973.7, 100127, 100370, 100049
> >
> > + 16 Entry Alignment
> > New: 129558, 129916, 122373, 124056
> > Old: 125361, 96475.4, 97457.8, 124319
> >
> > + 32 Entry Alignment
> > New: 95073.7, 92857.8, 90182.2, 92666.3
> > Old: 96702.1, 98558.9, 96797.1, 96887.1
> >
> > + 48 Entry Alignment
> > New: 135161, 134010, 123023, 148589
> > Old: 128150, 139029, 98382.2, 122686
> >
> > So the 32 byte/64 byte entry alignment versions seem to favor this
> change,
> > but when entry alignment % 16 != 0 this change seems to perform worse.
> >
> > I generally think until we understand why the byte padding is necessary
> its
> > probably a mistake to include it unless its a benefit in actual SPEC2017.
> >
> > My general intuition is that this is an issue with the benchmarks
> themselves
> > so I support the change w.o the padding.
>
> Here is the v3 patch without the padding. OK for master?
>
Ok with this change going out.
But I think we need to find the root cause of this degradation before more
changes are made to the file. At the moment just about any change that adds
25 bytes before L(less_vec) will look like a major win.
>
> >
> >> 2. Update comments for __x86_string_control.
> >>
> >>
> >> --
> >> H.J.
>
>
>
> --
> H.J.
>
On Tue, Jul 27, 2021 at 12:50 PM Noah Goldstein <goldstein.w.n@gmail.com> wrote:
>
>
>
> On Tue, Jul 27, 2021 at 3:23 PM H.J. Lu <hjl.tools@gmail.com> wrote:
>>
>> On Tue, Jul 27, 2021 at 12:12 PM Noah Goldstein <goldstein.w.n@gmail.com> wrote:
>> >
>> >
>> >
>> > On Tue, Jul 27, 2021 at 12:06 PM H.J. Lu <hjl.tools@gmail.com> wrote:
>> >>
>> >> On Mon, Jul 26, 2021 at 9:06 PM Noah Goldstein <goldstein.w.n@gmail.com> wrote:
>> >> >
>> >> >
>> >> >
>> >> > On Mon, Jul 26, 2021 at 11:11 PM H.J. Lu via Libc-alpha <libc-alpha@sourceware.org> wrote:
>> >> >>
>> >> >> On Mon, Jul 26, 2021 at 7:15 PM Carlos O'Donell <carlos@redhat.com> wrote:
>> >> >> >
>> >> >> > On 7/26/21 8:00 AM, H.J. Lu via Libc-alpha wrote:
>> >> >> > > commit 3ec5d83d2a237d39e7fd6ef7a0bc8ac4c171a4a5
>> >> >> > > Author: H.J. Lu <hjl.tools@gmail.com>
>> >> >> > > Date: Sat Jan 25 14:19:40 2020 -0800
>> >> >> > >
>> >> >> > > x86-64: Avoid rep movsb with short distance [BZ #27130]
>> >> >> > > introduced some regressions on Intel processors without Fast Short REP
>> >> >> > > MOV (FSRM). Add Avoid_Short_Distance_REP_MOVSB to avoid rep movsb with
>> >> >> > > short distance only on Intel processors with FSRM. bench-memmove-large
>> >> >> > > on Skylake server shows that cycles of __memmove_evex_unaligned_erms are
>> >> >> > > improved for the following data size:
>> >> >> > >
>> >> >> > > before after Improvement
>> >> >> > > length=4127, align1=3, align2=0: 479.38 343.00 28%
>> >> >> > > length=4223, align1=9, align2=5: 405.62 335.50 17%
>> >> >> > > length=8223, align1=3, align2=0: 786.12 495.00 37%
>> >> >> > > length=8319, align1=9, align2=5: 256.69 170.38 33%
>> >> >> > > length=16415, align1=3, align2=0: 1436.88 839.50 41%
>> >> >> > > length=16511, align1=9, align2=5: 1375.50 840.62 39%
>> >> >> > > length=32799, align1=3, align2=0: 2890.00 1850.62 36%
>> >> >> > > length=32895, align1=9, align2=5: 2891.38 1948.62 32%
>> >> >> > >
>> >> >> > > There are no regression on Ice Lake server.
>> >> >> >
>> >> >> > At this point we're waiting on Noah to provide feedback on the performance
>> >> >> > results given the alignment nop insertion you provided as a follow-up patch
>> >> >
>> >> >
>> >> > The results with the padding look good!
>> >> >
>> >> >>
>> >> >>
>> >> >> We are testing 25 byte nop padding now:
>> >> >>
>> >> >>
>> >> >> https://gitlab.com/x86-glibc/glibc/-/commit/de8985640a568786a59576716db54e0749d420e8
>> >> >>
>> >> > How did you come to the exact padding choice used?
>> >>
>> >> I first replaced the 9 byte instructions:
>> >>
>> >> andl $X86_STRING_CONTROL_AVOID_SHORT_DISTANCE_REP_MOVSB,
>> >> __x86_string_control(%rip)
>> >> jz 3f
>> >>
>> >> with a 9-byte NOP and reproduced the regression on Tiger Lake. It confirmed
>> >> that the code layout caused the regression. I first tried adding
>> >> ".p2align 4" to
>> >> branch targets and they made no differences. Then I started adding different
>> >> size of nops after
>> >>
>> >> andl $X86_STRING_CONTROL_AVOID_SHORT_DISTANCE_REP_MOVSB,
>> >> __x86_string_control(%rip)
>> >> jz 3f
>> >> movq %rdi, %rcx
>> >> subq %rsi, %rcx
>> >> jmp 2f
>> >>
>> >> with ".nops N". I started with N == 1 and doubled N in each step. I
>> >> noticed that
>> >> improvement started at N == 32. I started bisecting between 16 and 32:
>> >>
>> >> 1. 24 and 32 are good.
>> >> 2. 24 and 28 are good.
>> >> 3. 25 is the best overall.
>> >>
>> >> >>
>> >> >> > (unless you can confirm this yourself).
>> >> >> >
>> >> >> > Looking forward to a v2 the incorporates the alignment fix (pending Noah's
>> >> >> > comments), and my suggestions below.
>> >> >>
>> >> >> >
>> >> >> > > ---
>> >> >> > > sysdeps/x86/cacheinfo.h | 7 +++++++
>> >> >> > > sysdeps/x86/cpu-features.c | 5 +++++
>> >> >> > > .../x86/include/cpu-features-preferred_feature_index_1.def | 1 +
>> >> >> > > sysdeps/x86/sysdep.h | 3 +++
>> >> >> > > sysdeps/x86_64/multiarch/memmove-vec-unaligned-erms.S | 5 +++++
>> >> >> > > 5 files changed, 21 insertions(+)
>> >> >> > >
>> >> >> > > diff --git a/sysdeps/x86/cacheinfo.h b/sysdeps/x86/cacheinfo.h
>> >> >> > > index eba8dbc4a6..174ea38f5b 100644
>> >> >> > > --- a/sysdeps/x86/cacheinfo.h
>> >> >> > > +++ b/sysdeps/x86/cacheinfo.h
>> >> >> > > @@ -49,6 +49,9 @@ long int __x86_rep_stosb_threshold attribute_hidden = 2048;
>> >> >> > > /* Threshold to stop using Enhanced REP MOVSB. */
>> >> >> > > long int __x86_rep_movsb_stop_threshold attribute_hidden;
>> >> >> > >
>> >> >> > > +/* String/memory function control. */
>> >> >> > > +int __x86_string_control attribute_hidden;
>> >> >> >
>> >> >> > Please expand comment.
>> >> >> >
>> >> >> > Suggest:
>> >> >> >
>> >> >> > /* A bit-wise OR of string/memory requirements for optimal performance
>> >> >> > e.g. X86_STRING_CONTROL_AVOID_SHORT_DISTANCE_REP_MOVSB. These bits
>> >> >> > are used at runtime to tune implementation behavior. */
>> >> >> > int __x86_string_control attribute_hidden;
>> >> >>
>> >> >> I will fix it in the v2 patch.
>> >> >>
>> >> >> Thanks.
>> >> >>
>> >> >> > > +
>> >> >> > > static void
>> >> >> > > init_cacheinfo (void)
>> >> >> > > {
>> >> >> > > @@ -71,5 +74,9 @@ init_cacheinfo (void)
>> >> >> > > __x86_rep_movsb_threshold = cpu_features->rep_movsb_threshold;
>> >> >> > > __x86_rep_stosb_threshold = cpu_features->rep_stosb_threshold;
>> >> >> > > __x86_rep_movsb_stop_threshold = cpu_features->rep_movsb_stop_threshold;
>> >> >> > > +
>> >> >> > > + if (CPU_FEATURES_ARCH_P (cpu_features, Avoid_Short_Distance_REP_MOVSB))
>> >> >> > > + __x86_string_control
>> >> >> > > + |= X86_STRING_CONTROL_AVOID_SHORT_DISTANCE_REP_MOVSB;
>> >> >> >
>> >> >> > OK.
>> >> >> >
>> >> >> > > }
>> >> >> > > #endif
>> >> >> > > diff --git a/sysdeps/x86/cpu-features.c b/sysdeps/x86/cpu-features.c
>> >> >> > > index 706a172ba9..645bba6314 100644
>> >> >> > > --- a/sysdeps/x86/cpu-features.c
>> >> >> > > +++ b/sysdeps/x86/cpu-features.c
>> >> >> > > @@ -555,6 +555,11 @@ init_cpu_features (struct cpu_features *cpu_features)
>> >> >> > > cpu_features->preferred[index_arch_Prefer_AVX2_STRCMP]
>> >> >> > > |= bit_arch_Prefer_AVX2_STRCMP;
>> >> >> > > }
>> >> >> > > +
>> >> >> > > + /* Avoid avoid short distance REP MOVSB on processor with FSRM. */
>> >> >> > > + if (CPU_FEATURES_CPU_P (cpu_features, FSRM))
>> >> >> > > + cpu_features->preferred[index_arch_Avoid_Short_Distance_REP_MOVSB]
>> >> >> > > + |= bit_arch_Avoid_Short_Distance_REP_MOVSB;
>> >> >> >
>> >> >> > OK.
>> >> >> >
>> >> >> > > }
>> >> >> > > /* This spells out "AuthenticAMD" or "HygonGenuine". */
>> >> >> > > else if ((ebx == 0x68747541 && ecx == 0x444d4163 && edx == 0x69746e65)
>> >> >> > > diff --git a/sysdeps/x86/include/cpu-features-preferred_feature_index_1.def b/sysdeps/x86/include/cpu-features-preferred_feature_index_1.def
>> >> >> > > index 133aab19f1..d7c93f00c5 100644
>> >> >> > > --- a/sysdeps/x86/include/cpu-features-preferred_feature_index_1.def
>> >> >> > > +++ b/sysdeps/x86/include/cpu-features-preferred_feature_index_1.def
>> >> >> > > @@ -33,3 +33,4 @@ BIT (Prefer_No_AVX512)
>> >> >> > > BIT (MathVec_Prefer_No_AVX512)
>> >> >> > > BIT (Prefer_FSRM)
>> >> >> > > BIT (Prefer_AVX2_STRCMP)
>> >> >> > > +BIT (Avoid_Short_Distance_REP_MOVSB)
>> >> >> >
>> >> >> > OK.
>> >> >> >
>> >> >> > > diff --git a/sysdeps/x86/sysdep.h b/sysdeps/x86/sysdep.h
>> >> >> > > index 51c069bfe1..35cb90d507 100644
>> >> >> > > --- a/sysdeps/x86/sysdep.h
>> >> >> > > +++ b/sysdeps/x86/sysdep.h
>> >> >> > > @@ -57,6 +57,9 @@ enum cf_protection_level
>> >> >> > > #define STATE_SAVE_MASK \
>> >> >> > > ((1 << 1) | (1 << 2) | (1 << 3) | (1 << 5) | (1 << 6) | (1 << 7))
>> >> >> > >
>> >> >> >
>> >> >> > Suggest adding:
>> >> >> >
>> >> >> > /* Constants for bits in __x86_string_control: */
>> >> >> >
>> >> >> > > +/* Avoid short distance REP MOVSB. */
>> >> >> > > +#define X86_STRING_CONTROL_AVOID_SHORT_DISTANCE_REP_MOVSB (1 << 0)
>> >> >> >
>> >> >> > OK.
>> >> >> >
>> >> >> > > +
>> >> >> > > #ifdef __ASSEMBLER__
>> >> >> > >
>> >> >> > > /* Syntactic details of assembler. */
>> >> >> > > diff --git a/sysdeps/x86_64/multiarch/memmove-vec-unaligned-erms.S b/sysdeps/x86_64/multiarch/memmove-vec-unaligned-erms.S
>> >> >> > > index a783da5de2..9f02624375 100644
>> >> >> > > --- a/sysdeps/x86_64/multiarch/memmove-vec-unaligned-erms.S
>> >> >> > > +++ b/sysdeps/x86_64/multiarch/memmove-vec-unaligned-erms.S
>> >> >> > > @@ -325,12 +325,16 @@ L(movsb):
>> >> >> > > /* Avoid slow backward REP MOVSB. */
>> >> >> > > jb L(more_8x_vec_backward)
>> >> >> > > # if AVOID_SHORT_DISTANCE_REP_MOVSB
>> >> >> > > + andl $X86_STRING_CONTROL_AVOID_SHORT_DISTANCE_REP_MOVSB, __x86_string_control(%rip)
>> >> >> > > + jz 3f
>> >> >> >
>> >> >> > OK.
>> >> >> >
>> >> >> > > movq %rdi, %rcx
>> >> >> > > subq %rsi, %rcx
>> >> >> > > jmp 2f
>> >> >> > > # endif
>> >> >> > > 1:
>> >> >> > > # if AVOID_SHORT_DISTANCE_REP_MOVSB
>> >> >> > > + andl $X86_STRING_CONTROL_AVOID_SHORT_DISTANCE_REP_MOVSB, __x86_string_control(%rip)
>> >> >> > > + jz 3f
>> >> >> >
>> >> >> > OK.
>> >> >> >
>> >> >> > > movq %rsi, %rcx
>> >> >> > > subq %rdi, %rcx
>> >> >> > > 2:
>> >> >> > > @@ -338,6 +342,7 @@ L(movsb):
>> >> >> > > is N*4GB + [1..63] with N >= 0. */
>> >> >> > > cmpl $63, %ecx
>> >> >> > > jbe L(more_2x_vec) /* Avoid "rep movsb" if ECX <= 63. */
>> >> >> > > +3:
>> >> >> >
>> >> >> > OK.
>> >> >> >
>> >> >> > > # endif
>> >> >> > > mov %RDX_LP, %RCX_LP
>> >> >> > > rep movsb
>> >> >> > >
>> >> >> >
>> >> >> >
>> >> >> > --
>> >> >> > Cheers,
>> >> >> > Carlos.
>> >> >> >
>> >> >>
>> >> >>
>> >> >> --
>> >> >> H.J.
>> >>
>> >> Here is the v2 patch:
>> >>
>> >> 1. Add a 25-byte NOP padding after JMP for Avoid_Short_Distance_REP_MOVSB,
>> >> which improves bench-memcpy-random performance on Tiger Lake by ~30%
>> >
>> >
>> > I think this may not be due to unrelated factors. I reran the random benchmarks with
>> > the function on a fresh page and entry of the *_erms version at either + 0, 16, 32, 48
>> > bytes and 1) don't see a universal improvement and 2) believe it's likely that the 30%
>> > you measured is due to unrelated alignment changes.
>> >
>> > "__memcpy_avx_unaligned", "__memcpy_avx_unaligned_erms", "__memcpy_evex_unaligned", "__memcpy_evex_unaligned_erms"
>> >
>> > + 0 Entry Alignment
>> > New: 91824.1, 95460.1, 95063.3, 97998.3
>> > Old: 99973.7, 100127, 100370, 100049
>> >
>> > + 16 Entry Alignment
>> > New: 129558, 129916, 122373, 124056
>> > Old: 125361, 96475.4, 97457.8, 124319
>> >
>> > + 32 Entry Alignment
>> > New: 95073.7, 92857.8, 90182.2, 92666.3
>> > Old: 96702.1, 98558.9, 96797.1, 96887.1
>> >
>> > + 48 Entry Alignment
>> > New: 135161, 134010, 123023, 148589
>> > Old: 128150, 139029, 98382.2, 122686
>> >
>> > So the 32 byte/64 byte entry alignment versions seem to favor this change,
>> > but when entry alignment % 16 != 0 this change seems to perform worse.
>> >
>> > I generally think until we understand why the byte padding is necessary its
>> > probably a mistake to include it unless its a benefit in actual SPEC2017.
>> >
>> > My general intuition is that this is an issue with the benchmarks themselves
>> > so I support the change w.o the padding.
>>
>> Here is the v3 patch without the padding. OK for master?
>
>
> Ok with this change going out.
>
> But I think we need to find the root cause of this degradation before more
> changes are made to the file. At the moment just about any change that adds
> 25 bytes before L(less_vec) will look like a major win.
Dianhong, please run vtune to investigate why a 25-byte padding makes such
a difference. Thanks.
>>
>>
>> >
>> >> 2. Update comments for __x86_string_control.
>> >>
>> >>
>> >> --
>> >> H.J.
>>
>>
>>
>> --
>> H.J.
On Tue, Jul 27, 2021 at 12:50 PM Noah Goldstein <goldstein.w.n@gmail.com> wrote:
>
>
>
> On Tue, Jul 27, 2021 at 3:23 PM H.J. Lu <hjl.tools@gmail.com> wrote:
>>
>> On Tue, Jul 27, 2021 at 12:12 PM Noah Goldstein <goldstein.w.n@gmail.com> wrote:
>> >
>> >
>> >
>> > On Tue, Jul 27, 2021 at 12:06 PM H.J. Lu <hjl.tools@gmail.com> wrote:
>> >>
>> >> On Mon, Jul 26, 2021 at 9:06 PM Noah Goldstein <goldstein.w.n@gmail.com> wrote:
>> >> >
>> >> >
>> >> >
>> >> > On Mon, Jul 26, 2021 at 11:11 PM H.J. Lu via Libc-alpha <libc-alpha@sourceware.org> wrote:
>> >> >>
>> >> >> On Mon, Jul 26, 2021 at 7:15 PM Carlos O'Donell <carlos@redhat.com> wrote:
>> >> >> >
>> >> >> > On 7/26/21 8:00 AM, H.J. Lu via Libc-alpha wrote:
>> >> >> > > commit 3ec5d83d2a237d39e7fd6ef7a0bc8ac4c171a4a5
>> >> >> > > Author: H.J. Lu <hjl.tools@gmail.com>
>> >> >> > > Date: Sat Jan 25 14:19:40 2020 -0800
>> >> >> > >
>> >> >> > > x86-64: Avoid rep movsb with short distance [BZ #27130]
>> >> >> > > introduced some regressions on Intel processors without Fast Short REP
>> >> >> > > MOV (FSRM). Add Avoid_Short_Distance_REP_MOVSB to avoid rep movsb with
>> >> >> > > short distance only on Intel processors with FSRM. bench-memmove-large
>> >> >> > > on Skylake server shows that cycles of __memmove_evex_unaligned_erms are
>> >> >> > > improved for the following data size:
>> >> >> > >
>> >> >> > > before after Improvement
>> >> >> > > length=4127, align1=3, align2=0: 479.38 343.00 28%
>> >> >> > > length=4223, align1=9, align2=5: 405.62 335.50 17%
>> >> >> > > length=8223, align1=3, align2=0: 786.12 495.00 37%
>> >> >> > > length=8319, align1=9, align2=5: 256.69 170.38 33%
>> >> >> > > length=16415, align1=3, align2=0: 1436.88 839.50 41%
>> >> >> > > length=16511, align1=9, align2=5: 1375.50 840.62 39%
>> >> >> > > length=32799, align1=3, align2=0: 2890.00 1850.62 36%
>> >> >> > > length=32895, align1=9, align2=5: 2891.38 1948.62 32%
>> >> >> > >
>> >> >> > > There are no regression on Ice Lake server.
>> >> >> >
>> >> >> > At this point we're waiting on Noah to provide feedback on the performance
>> >> >> > results given the alignment nop insertion you provided as a follow-up patch
>> >> >
>> >> >
>> >> > The results with the padding look good!
>> >> >
>> >> >>
>> >> >>
>> >> >> We are testing 25 byte nop padding now:
>> >> >>
>> >> >>
>> >> >> https://gitlab.com/x86-glibc/glibc/-/commit/de8985640a568786a59576716db54e0749d420e8
>> >> >>
>> >> > How did you come to the exact padding choice used?
>> >>
>> >> I first replaced the 9 byte instructions:
>> >>
>> >> andl $X86_STRING_CONTROL_AVOID_SHORT_DISTANCE_REP_MOVSB,
>> >> __x86_string_control(%rip)
>> >> jz 3f
>> >>
>> >> with a 9-byte NOP and reproduced the regression on Tiger Lake. It confirmed
>> >> that the code layout caused the regression. I first tried adding
>> >> ".p2align 4" to
>> >> branch targets and they made no differences. Then I started adding different
>> >> size of nops after
>> >>
>> >> andl $X86_STRING_CONTROL_AVOID_SHORT_DISTANCE_REP_MOVSB,
>> >> __x86_string_control(%rip)
>> >> jz 3f
>> >> movq %rdi, %rcx
>> >> subq %rsi, %rcx
>> >> jmp 2f
>> >>
>> >> with ".nops N". I started with N == 1 and doubled N in each step. I
>> >> noticed that
>> >> improvement started at N == 32. I started bisecting between 16 and 32:
>> >>
>> >> 1. 24 and 32 are good.
>> >> 2. 24 and 28 are good.
>> >> 3. 25 is the best overall.
>> >>
>> >> >>
>> >> >> > (unless you can confirm this yourself).
>> >> >> >
>> >> >> > Looking forward to a v2 the incorporates the alignment fix (pending Noah's
>> >> >> > comments), and my suggestions below.
>> >> >>
>> >> >> >
>> >> >> > > ---
>> >> >> > > sysdeps/x86/cacheinfo.h | 7 +++++++
>> >> >> > > sysdeps/x86/cpu-features.c | 5 +++++
>> >> >> > > .../x86/include/cpu-features-preferred_feature_index_1.def | 1 +
>> >> >> > > sysdeps/x86/sysdep.h | 3 +++
>> >> >> > > sysdeps/x86_64/multiarch/memmove-vec-unaligned-erms.S | 5 +++++
>> >> >> > > 5 files changed, 21 insertions(+)
>> >> >> > >
>> >> >> > > diff --git a/sysdeps/x86/cacheinfo.h b/sysdeps/x86/cacheinfo.h
>> >> >> > > index eba8dbc4a6..174ea38f5b 100644
>> >> >> > > --- a/sysdeps/x86/cacheinfo.h
>> >> >> > > +++ b/sysdeps/x86/cacheinfo.h
>> >> >> > > @@ -49,6 +49,9 @@ long int __x86_rep_stosb_threshold attribute_hidden = 2048;
>> >> >> > > /* Threshold to stop using Enhanced REP MOVSB. */
>> >> >> > > long int __x86_rep_movsb_stop_threshold attribute_hidden;
>> >> >> > >
>> >> >> > > +/* String/memory function control. */
>> >> >> > > +int __x86_string_control attribute_hidden;
>> >> >> >
>> >> >> > Please expand comment.
>> >> >> >
>> >> >> > Suggest:
>> >> >> >
>> >> >> > /* A bit-wise OR of string/memory requirements for optimal performance
>> >> >> > e.g. X86_STRING_CONTROL_AVOID_SHORT_DISTANCE_REP_MOVSB. These bits
>> >> >> > are used at runtime to tune implementation behavior. */
>> >> >> > int __x86_string_control attribute_hidden;
>> >> >>
>> >> >> I will fix it in the v2 patch.
>> >> >>
>> >> >> Thanks.
>> >> >>
>> >> >> > > +
>> >> >> > > static void
>> >> >> > > init_cacheinfo (void)
>> >> >> > > {
>> >> >> > > @@ -71,5 +74,9 @@ init_cacheinfo (void)
>> >> >> > > __x86_rep_movsb_threshold = cpu_features->rep_movsb_threshold;
>> >> >> > > __x86_rep_stosb_threshold = cpu_features->rep_stosb_threshold;
>> >> >> > > __x86_rep_movsb_stop_threshold = cpu_features->rep_movsb_stop_threshold;
>> >> >> > > +
>> >> >> > > + if (CPU_FEATURES_ARCH_P (cpu_features, Avoid_Short_Distance_REP_MOVSB))
>> >> >> > > + __x86_string_control
>> >> >> > > + |= X86_STRING_CONTROL_AVOID_SHORT_DISTANCE_REP_MOVSB;
>> >> >> >
>> >> >> > OK.
>> >> >> >
>> >> >> > > }
>> >> >> > > #endif
>> >> >> > > diff --git a/sysdeps/x86/cpu-features.c b/sysdeps/x86/cpu-features.c
>> >> >> > > index 706a172ba9..645bba6314 100644
>> >> >> > > --- a/sysdeps/x86/cpu-features.c
>> >> >> > > +++ b/sysdeps/x86/cpu-features.c
>> >> >> > > @@ -555,6 +555,11 @@ init_cpu_features (struct cpu_features *cpu_features)
>> >> >> > > cpu_features->preferred[index_arch_Prefer_AVX2_STRCMP]
>> >> >> > > |= bit_arch_Prefer_AVX2_STRCMP;
>> >> >> > > }
>> >> >> > > +
>> >> >> > > + /* Avoid avoid short distance REP MOVSB on processor with FSRM. */
>> >> >> > > + if (CPU_FEATURES_CPU_P (cpu_features, FSRM))
>> >> >> > > + cpu_features->preferred[index_arch_Avoid_Short_Distance_REP_MOVSB]
>> >> >> > > + |= bit_arch_Avoid_Short_Distance_REP_MOVSB;
>> >> >> >
>> >> >> > OK.
>> >> >> >
>> >> >> > > }
>> >> >> > > /* This spells out "AuthenticAMD" or "HygonGenuine". */
>> >> >> > > else if ((ebx == 0x68747541 && ecx == 0x444d4163 && edx == 0x69746e65)
>> >> >> > > diff --git a/sysdeps/x86/include/cpu-features-preferred_feature_index_1.def b/sysdeps/x86/include/cpu-features-preferred_feature_index_1.def
>> >> >> > > index 133aab19f1..d7c93f00c5 100644
>> >> >> > > --- a/sysdeps/x86/include/cpu-features-preferred_feature_index_1.def
>> >> >> > > +++ b/sysdeps/x86/include/cpu-features-preferred_feature_index_1.def
>> >> >> > > @@ -33,3 +33,4 @@ BIT (Prefer_No_AVX512)
>> >> >> > > BIT (MathVec_Prefer_No_AVX512)
>> >> >> > > BIT (Prefer_FSRM)
>> >> >> > > BIT (Prefer_AVX2_STRCMP)
>> >> >> > > +BIT (Avoid_Short_Distance_REP_MOVSB)
>> >> >> >
>> >> >> > OK.
>> >> >> >
>> >> >> > > diff --git a/sysdeps/x86/sysdep.h b/sysdeps/x86/sysdep.h
>> >> >> > > index 51c069bfe1..35cb90d507 100644
>> >> >> > > --- a/sysdeps/x86/sysdep.h
>> >> >> > > +++ b/sysdeps/x86/sysdep.h
>> >> >> > > @@ -57,6 +57,9 @@ enum cf_protection_level
>> >> >> > > #define STATE_SAVE_MASK \
>> >> >> > > ((1 << 1) | (1 << 2) | (1 << 3) | (1 << 5) | (1 << 6) | (1 << 7))
>> >> >> > >
>> >> >> >
>> >> >> > Suggest adding:
>> >> >> >
>> >> >> > /* Constants for bits in __x86_string_control: */
>> >> >> >
>> >> >> > > +/* Avoid short distance REP MOVSB. */
>> >> >> > > +#define X86_STRING_CONTROL_AVOID_SHORT_DISTANCE_REP_MOVSB (1 << 0)
>> >> >> >
>> >> >> > OK.
>> >> >> >
>> >> >> > > +
>> >> >> > > #ifdef __ASSEMBLER__
>> >> >> > >
>> >> >> > > /* Syntactic details of assembler. */
>> >> >> > > diff --git a/sysdeps/x86_64/multiarch/memmove-vec-unaligned-erms.S b/sysdeps/x86_64/multiarch/memmove-vec-unaligned-erms.S
>> >> >> > > index a783da5de2..9f02624375 100644
>> >> >> > > --- a/sysdeps/x86_64/multiarch/memmove-vec-unaligned-erms.S
>> >> >> > > +++ b/sysdeps/x86_64/multiarch/memmove-vec-unaligned-erms.S
>> >> >> > > @@ -325,12 +325,16 @@ L(movsb):
>> >> >> > > /* Avoid slow backward REP MOVSB. */
>> >> >> > > jb L(more_8x_vec_backward)
>> >> >> > > # if AVOID_SHORT_DISTANCE_REP_MOVSB
>> >> >> > > + andl $X86_STRING_CONTROL_AVOID_SHORT_DISTANCE_REP_MOVSB, __x86_string_control(%rip)
>> >> >> > > + jz 3f
>> >> >> >
>> >> >> > OK.
>> >> >> >
>> >> >> > > movq %rdi, %rcx
>> >> >> > > subq %rsi, %rcx
>> >> >> > > jmp 2f
>> >> >> > > # endif
>> >> >> > > 1:
>> >> >> > > # if AVOID_SHORT_DISTANCE_REP_MOVSB
>> >> >> > > + andl $X86_STRING_CONTROL_AVOID_SHORT_DISTANCE_REP_MOVSB, __x86_string_control(%rip)
>> >> >> > > + jz 3f
>> >> >> >
>> >> >> > OK.
>> >> >> >
>> >> >> > > movq %rsi, %rcx
>> >> >> > > subq %rdi, %rcx
>> >> >> > > 2:
>> >> >> > > @@ -338,6 +342,7 @@ L(movsb):
>> >> >> > > is N*4GB + [1..63] with N >= 0. */
>> >> >> > > cmpl $63, %ecx
>> >> >> > > jbe L(more_2x_vec) /* Avoid "rep movsb" if ECX <= 63. */
>> >> >> > > +3:
>> >> >> >
>> >> >> > OK.
>> >> >> >
>> >> >> > > # endif
>> >> >> > > mov %RDX_LP, %RCX_LP
>> >> >> > > rep movsb
>> >> >> > >
>> >> >> >
>> >> >> >
>> >> >> > --
>> >> >> > Cheers,
>> >> >> > Carlos.
>> >> >> >
>> >> >>
>> >> >>
>> >> >> --
>> >> >> H.J.
>> >>
>> >> Here is the v2 patch:
>> >>
>> >> 1. Add a 25-byte NOP padding after JMP for Avoid_Short_Distance_REP_MOVSB,
>> >> which improves bench-memcpy-random performance on Tiger Lake by ~30%
>> >
>> >
>> > I think this may not be due to unrelated factors. I reran the random benchmarks with
>> > the function on a fresh page and entry of the *_erms version at either + 0, 16, 32, 48
>> > bytes and 1) don't see a universal improvement and 2) believe it's likely that the 30%
>> > you measured is due to unrelated alignment changes.
>> >
>> > "__memcpy_avx_unaligned", "__memcpy_avx_unaligned_erms", "__memcpy_evex_unaligned", "__memcpy_evex_unaligned_erms"
>> >
>> > + 0 Entry Alignment
>> > New: 91824.1, 95460.1, 95063.3, 97998.3
>> > Old: 99973.7, 100127, 100370, 100049
>> >
>> > + 16 Entry Alignment
>> > New: 129558, 129916, 122373, 124056
>> > Old: 125361, 96475.4, 97457.8, 124319
>> >
>> > + 32 Entry Alignment
>> > New: 95073.7, 92857.8, 90182.2, 92666.3
>> > Old: 96702.1, 98558.9, 96797.1, 96887.1
>> >
>> > + 48 Entry Alignment
>> > New: 135161, 134010, 123023, 148589
>> > Old: 128150, 139029, 98382.2, 122686
>> >
>> > So the 32 byte/64 byte entry alignment versions seem to favor this change,
>> > but when entry alignment % 16 != 0 this change seems to perform worse.
>> >
>> > I generally think until we understand why the byte padding is necessary its
>> > probably a mistake to include it unless its a benefit in actual SPEC2017.
>> >
>> > My general intuition is that this is an issue with the benchmarks themselves
>> > so I support the change w.o the padding.
>>
>> Here is the v3 patch without the padding. OK for master?
>
>
> Ok with this change going out.
I am going to check this version later today.
Thanks.
> But I think we need to find the root cause of this degradation before more
> changes are made to the file. At the moment just about any change that adds
> 25 bytes before L(less_vec) will look like a major win.
>>
>>
>> >
>> >> 2. Update comments for __x86_string_control.
>> >>
>> >>
>> >> --
>> >> H.J.
>>
>>
>>
>> --
>> H.J.
From 8ae1b914e8c0e0f58249937a2f348c9b297f70f6 Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@gmail.com>
Date: Thu, 22 Jul 2021 20:26:25 -0700
Subject: [PATCH v3] x86-64: Add Avoid_Short_Distance_REP_MOVSB
commit 3ec5d83d2a237d39e7fd6ef7a0bc8ac4c171a4a5
Author: H.J. Lu <hjl.tools@gmail.com>
Date: Sat Jan 25 14:19:40 2020 -0800
x86-64: Avoid rep movsb with short distance [BZ #27130]
introduced some regressions on Intel processors without Fast Short REP
MOV (FSRM). Add Avoid_Short_Distance_REP_MOVSB to avoid rep movsb with
short distance only on Intel processors with FSRM. bench-memmove-large
on Skylake server shows that cycles of __memmove_evex_unaligned_erms
improves for the following data size:
before after Improvement
length=4127, align1=3, align2=0: 479.38 349.25 27%
length=4223, align1=9, align2=5: 405.62 333.25 18%
length=8223, align1=3, align2=0: 786.12 496.38 37%
length=8319, align1=9, align2=5: 727.50 501.38 31%
length=16415, align1=3, align2=0: 1436.88 840.00 41%
length=16511, align1=9, align2=5: 1375.50 836.38 39%
length=32799, align1=3, align2=0: 2890.00 1860.12 36%
length=32895, align1=9, align2=5: 2891.38 1931.88 33%
---
sysdeps/x86/cacheinfo.h | 9 +++++++++
sysdeps/x86/cpu-features.c | 5 +++++
.../include/cpu-features-preferred_feature_index_1.def | 1 +
sysdeps/x86/sysdep.h | 5 +++++
sysdeps/x86_64/multiarch/memmove-vec-unaligned-erms.S | 5 +++++
5 files changed, 25 insertions(+)
@@ -49,6 +49,11 @@ long int __x86_rep_stosb_threshold attribute_hidden = 2048;
/* Threshold to stop using Enhanced REP MOVSB. */
long int __x86_rep_movsb_stop_threshold attribute_hidden;
+/* A bit-wise OR of string/memory requirements for optimal performance
+ e.g. X86_STRING_CONTROL_AVOID_SHORT_DISTANCE_REP_MOVSB. These bits
+ are used at runtime to tune implementation behavior. */
+int __x86_string_control attribute_hidden;
+
static void
init_cacheinfo (void)
{
@@ -71,5 +76,9 @@ init_cacheinfo (void)
__x86_rep_movsb_threshold = cpu_features->rep_movsb_threshold;
__x86_rep_stosb_threshold = cpu_features->rep_stosb_threshold;
__x86_rep_movsb_stop_threshold = cpu_features->rep_movsb_stop_threshold;
+
+ if (CPU_FEATURES_ARCH_P (cpu_features, Avoid_Short_Distance_REP_MOVSB))
+ __x86_string_control
+ |= X86_STRING_CONTROL_AVOID_SHORT_DISTANCE_REP_MOVSB;
}
#endif
@@ -555,6 +555,11 @@ init_cpu_features (struct cpu_features *cpu_features)
cpu_features->preferred[index_arch_Prefer_AVX2_STRCMP]
|= bit_arch_Prefer_AVX2_STRCMP;
}
+
+ /* Avoid avoid short distance REP MOVSB on processor with FSRM. */
+ if (CPU_FEATURES_CPU_P (cpu_features, FSRM))
+ cpu_features->preferred[index_arch_Avoid_Short_Distance_REP_MOVSB]
+ |= bit_arch_Avoid_Short_Distance_REP_MOVSB;
}
/* This spells out "AuthenticAMD" or "HygonGenuine". */
else if ((ebx == 0x68747541 && ecx == 0x444d4163 && edx == 0x69746e65)
@@ -33,3 +33,4 @@ BIT (Prefer_No_AVX512)
BIT (MathVec_Prefer_No_AVX512)
BIT (Prefer_FSRM)
BIT (Prefer_AVX2_STRCMP)
+BIT (Avoid_Short_Distance_REP_MOVSB)
@@ -57,6 +57,11 @@ enum cf_protection_level
#define STATE_SAVE_MASK \
((1 << 1) | (1 << 2) | (1 << 3) | (1 << 5) | (1 << 6) | (1 << 7))
+/* Constants for bits in __x86_string_control: */
+
+/* Avoid short distance REP MOVSB. */
+#define X86_STRING_CONTROL_AVOID_SHORT_DISTANCE_REP_MOVSB (1 << 0)
+
#ifdef __ASSEMBLER__
/* Syntactic details of assembler. */
@@ -325,12 +325,16 @@ L(movsb):
/* Avoid slow backward REP MOVSB. */
jb L(more_8x_vec_backward)
# if AVOID_SHORT_DISTANCE_REP_MOVSB
+ andl $X86_STRING_CONTROL_AVOID_SHORT_DISTANCE_REP_MOVSB, __x86_string_control(%rip)
+ jz 3f
movq %rdi, %rcx
subq %rsi, %rcx
jmp 2f
# endif
1:
# if AVOID_SHORT_DISTANCE_REP_MOVSB
+ andl $X86_STRING_CONTROL_AVOID_SHORT_DISTANCE_REP_MOVSB, __x86_string_control(%rip)
+ jz 3f
movq %rsi, %rcx
subq %rdi, %rcx
2:
@@ -338,6 +342,7 @@ L(movsb):
is N*4GB + [1..63] with N >= 0. */
cmpl $63, %ecx
jbe L(more_2x_vec) /* Avoid "rep movsb" if ECX <= 63. */
+3:
# endif
mov %RDX_LP, %RCX_LP
rep movsb
--
2.31.1