diff mbox series

AArch64: Update A64FX memset not to degrade at 16KB

Message ID 20210827050304.543471-1-naohirot@fujitsu.com
State Committed
Commit 1d9f99ce1b3788d1897cb53a76d57e973111b8fe
Headers show
Series AArch64: Update A64FX memset not to degrade at 16KB | expand

Checks

Context Check Description
dj/TryBot-apply_patch success Patch applied to master at the time it was sent
dj/TryBot-32bit success Build for i686

Commit Message

Naohiro Tamura Aug. 27, 2021, 5:03 a.m. UTC
This patch updates unroll8 code so as not to degrade at the peak
performance 16KB for both FX1000 and FX700.

Inserted 2 instructions at the beginning of the unroll8 loop,
cmp and branch, are a workaround that is found heuristically.

Reviewed-by: Wilco Dijkstra <Wilco.Dijkstra@arm.com>
---
 sysdeps/aarch64/multiarch/memset_a64fx.S | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

Comments

Szabolcs Nagy Sept. 3, 2021, 3:01 p.m. UTC | #1
The 08/27/2021 05:03, Naohiro Tamura via Libc-alpha wrote:
> This patch updates unroll8 code so as not to degrade at the peak
> performance 16KB for both FX1000 and FX700.
> 
> Inserted 2 instructions at the beginning of the unroll8 loop,
> cmp and branch, are a workaround that is found heuristically.
> 
> Reviewed-by: Wilco Dijkstra <Wilco.Dijkstra@arm.com>

thanks, i committed this now.


> ---
>  sysdeps/aarch64/multiarch/memset_a64fx.S | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/sysdeps/aarch64/multiarch/memset_a64fx.S b/sysdeps/aarch64/multiarch/memset_a64fx.S
> index 7bf759b6a753..f7dfdaace7cf 100644
> --- a/sysdeps/aarch64/multiarch/memset_a64fx.S
> +++ b/sysdeps/aarch64/multiarch/memset_a64fx.S
> @@ -96,7 +96,14 @@ L(vl_agnostic): // VL Agnostic
>  L(unroll8):
>  	sub	count, count, tmp1
>  	.p2align 4
> -1:	st1b_unroll 0, 7
> +	// The 2 instructions at the beginning of the following loop,
> +	// cmp and branch, are a workaround so as not to degrade at
> +	// the peak performance 16KB.
> +	// It is found heuristically and the branch condition, b.ne,
> +	// is chosen intentionally never to jump.
> +1:	cmp	xzr, xzr
> +	b.ne	1b
> +	st1b_unroll 0, 7
>  	add	dst, dst, tmp1
>  	subs	count, count, tmp1
>  	b.hi	1b
> -- 
> 2.17.1
>
Florian Weimer Sept. 6, 2021, 6:17 a.m. UTC | #2
* naohirot:

> I noticed that my email address is not right in the commit log,
> would you please fix it to <naohirot@fujitsu.com>?
>
> commit 23777232c23f80809613bdfa329f63aadf992922
> Author: Naohiro Tamura via Libc-alpha <libc-alpha@sourceware.org>
> Date:   Fri Aug 27 05:03:04 2021 +0000

It's probably too late to do this now because it would mean rewriting
history.

Unfortunately, we do not have much control over the mailing list
configuration (to make “git am” work) or the commit hooks (to prevent
people from pushing such changes).  Sorry about that.

Florian
Guo, Wangyang via Libc-alpha Sept. 6, 2021, 8:44 a.m. UTC | #3
Hi Florian, Szabolcs, and All maintainers,

> From: Florian Weimer <fweimer@redhat.com>
> Sent: Monday, September 6, 2021 3:18 PM
> * naohirot:
> 
> > I noticed that my email address is not right in the commit log,
> > would you please fix it to <naohirot@fujitsu.com>?
> >
> > commit 23777232c23f80809613bdfa329f63aadf992922
> > Author: Naohiro Tamura via Libc-alpha <libc-alpha@sourceware.org>
> > Date:   Fri Aug 27 05:03:04 2021 +0000
> 
> It's probably too late to do this now because it would mean rewriting
> history.

Yes, I know that it's impossible update only email address in the commit log.
Currently there are two commits after the patch, so rewriting history involves
only three records. 
If rewriting history is not acceptable, I prefer to revert and recommit.

"Author: " line in the commit log is the identity for each contributor.
And it's related to the " Update to glibc copyright assignment policy" discussion.
So I'd like to hear other maintainer's opinion.

Thanks.
Naohiro
Guo, Wangyang via Libc-alpha Sept. 6, 2021, 10:20 a.m. UTC | #4
The 09/06/2021 08:44, naohirot@fujitsu.com wrote:
> Hi Florian, Szabolcs, and All maintainers,
> 
> > From: Florian Weimer <fweimer@redhat.com>
> > Sent: Monday, September 6, 2021 3:18 PM
> > * naohirot:
> > 
> > > I noticed that my email address is not right in the commit log,
> > > would you please fix it to <naohirot@fujitsu.com>?
> > >
> > > commit 23777232c23f80809613bdfa329f63aadf992922
> > > Author: Naohiro Tamura via Libc-alpha <libc-alpha@sourceware.org>
> > > Date:   Fri Aug 27 05:03:04 2021 +0000
> > 
> > It's probably too late to do this now because it would mean rewriting
> > history.
> 
> Yes, I know that it's impossible update only email address in the commit log.
> Currently there are two commits after the patch, so rewriting history involves
> only three records. 
> If rewriting history is not acceptable, I prefer to revert and recommit.
> 
> "Author: " line in the commit log is the identity for each contributor.
> And it's related to the " Update to glibc copyright assignment policy" discussion.
> So I'd like to hear other maintainer's opinion.

sorry i didnt notice.
normally we don't fix such mistakes,
but i reverted and recommitted this now.

i really wish the mailing list setting was fixed
not to clobber email addresses by default.
Guo, Wangyang via Libc-alpha Sept. 6, 2021, 11:56 a.m. UTC | #5
Hi Szabolcs,

> From: 'Szabolcs Nagy' <szabolcs.nagy@arm.com>
> Sent: Monday, September 6, 2021 7:21 PM
>
> sorry i didnt notice.
> normally we don't fix such mistakes,
> but i reverted and recommitted this now.

Thank you very much!
 
> i really wish the mailing list setting was fixed
> not to clobber email addresses by default.

Yes, I think so too.

Thanks.
Naohiro
Joseph Myers Sept. 6, 2021, 3:54 p.m. UTC | #6
On Mon, 6 Sep 2021, Florian Weimer via Libc-alpha wrote:

> Unfortunately, we do not have much control over the mailing list
> configuration (to make “git am” work) or the commit hooks (to prevent
> people from pushing such changes).  Sorry about that.

We do have sufficient control over the git hooks.  You can set 
commit-extra-checker (in refs/meta/config:project.config) to point to a 
script that applies such checks to commits.  See 
/git/gcc.git/hooks-bin/commit_checker for example; that includes code:

    # Reject commits applied via "git am" with list email address as
    # the author.
    if author_email in ('gcc-patches@gcc.gnu.org', 'libstdc++@gcc.gnu.org',
                        'fortran@gcc.gnu.org'):
        error('When applying a patch from a mailing list, make sure '
              'to use the original name and email address of the patch '
              'author, not the list email address with "via Gcc-patches".')

These checks are only applied to commits new to the repository, not 
commits new to a particular branch but already present in the repository, 
so applying stricter checks to new commits on particular branches can be 
trickier (see <https://github.com/AdaCore/git-hooks/issues/20>), but for 
this particular issue a branch-independent check should suffice.
Florian Weimer Sept. 16, 2021, 11:53 a.m. UTC | #7
* Joseph Myers:

> On Mon, 6 Sep 2021, Florian Weimer via Libc-alpha wrote:
>
>> Unfortunately, we do not have much control over the mailing list
>> configuration (to make “git am” work) or the commit hooks (to prevent
>> people from pushing such changes).  Sorry about that.
>
> We do have sufficient control over the git hooks.  You can set 
> commit-extra-checker (in refs/meta/config:project.config) to point to a 
> script that applies such checks to commits.  See 
> /git/gcc.git/hooks-bin/commit_checker for example; that includes code:
>
>     # Reject commits applied via "git am" with list email address as
>     # the author.
>     if author_email in ('gcc-patches@gcc.gnu.org', 'libstdc++@gcc.gnu.org',
>                         'fortran@gcc.gnu.org'):
>         error('When applying a patch from a mailing list, make sure '
>               'to use the original name and email address of the patch '
>               'author, not the list email address with "via Gcc-patches".')
>
> These checks are only applied to commits new to the repository, not 
> commits new to a particular branch but already present in the repository, 
> so applying stricter checks to new commits on particular branches can be 
> trickier (see <https://github.com/AdaCore/git-hooks/issues/20>), but for 
> this particular issue a branch-independent check should suffice.

I've tried to implement this now.  commit-extra-checker now points to a
trimmed-down version of the GCC script.

Thanks,
Florian
Guo, Wangyang via Libc-alpha Sept. 17, 2021, 12:49 a.m. UTC | #8
Hi Florian, Joseph,

> > These checks are only applied to commits new to the repository, not
> > commits new to a particular branch but already present in the repository,
> > so applying stricter checks to new commits on particular branches can be
> > trickier (see <https://github.com/AdaCore/git-hooks/issues/20>), but for
> > this particular issue a branch-independent check should suffice.
> 
> I've tried to implement this now.  commit-extra-checker now points to a
> trimmed-down version of the GCC script.

That's great!
Thank you for the update.
Naohiro
diff mbox series

Patch

diff --git a/sysdeps/aarch64/multiarch/memset_a64fx.S b/sysdeps/aarch64/multiarch/memset_a64fx.S
index 7bf759b6a753..f7dfdaace7cf 100644
--- a/sysdeps/aarch64/multiarch/memset_a64fx.S
+++ b/sysdeps/aarch64/multiarch/memset_a64fx.S
@@ -96,7 +96,14 @@  L(vl_agnostic): // VL Agnostic
 L(unroll8):
 	sub	count, count, tmp1
 	.p2align 4
-1:	st1b_unroll 0, 7
+	// The 2 instructions at the beginning of the following loop,
+	// cmp and branch, are a workaround so as not to degrade at
+	// the peak performance 16KB.
+	// It is found heuristically and the branch condition, b.ne,
+	// is chosen intentionally never to jump.
+1:	cmp	xzr, xzr
+	b.ne	1b
+	st1b_unroll 0, 7
 	add	dst, dst, tmp1
 	subs	count, count, tmp1
 	b.hi	1b