public inbox for libc-alpha@sourceware.org
 help / color / mirror / Atom feed
From: "Andrew Pinski (QUIC)" <quic_apinski@quicinc.com>
To: Florian Weimer <fweimer@redhat.com>,
	"Andrew Pinski (QUIC)" <quic_apinski@quicinc.com>
Cc: "libc-alpha@sourceware.org" <libc-alpha@sourceware.org>
Subject: RE: [PATCH v2 2/2] Aarch64: Add new memset for Qualcomm's 0ryon-1 core
Date: Tue, 14 May 2024 07:40:44 +0000	[thread overview]
Message-ID: <DM6PR02MB4058B6C2EEF2C3AF743FB575B8E32@DM6PR02MB4058.namprd02.prod.outlook.com> (raw)
In-Reply-To: <87ttj0vofm.fsf@oldenburg.str.redhat.com>

> -----Original Message-----
> From: Florian Weimer <fweimer@redhat.com>
> Sent: Tuesday, May 14, 2024 12:32 AM
> To: Andrew Pinski (QUIC) <quic_apinski@quicinc.com>
> Cc: libc-alpha@sourceware.org
> Subject: Re: [PATCH v2 2/2] Aarch64: Add new memset for Qualcomm's
> 0ryon-1 core
> 
> * Andrew Pinski:
> 
> > +L(try_zva):
> > +	/* Write the first and last 64 byte aligned block using stp rather
> > +	   than using DC ZVA.  This is faster on some cores.
> > +	 */
> 
> The “some cores” part seems outdated if it's just a memset for the
> Oryon-1 core (singulare).  This comment and some others, for example

Will fix.

> 
> > +	/*
> > +	 * Adjust count and bias for loop. By subtracting extra 1 from count,
> > +	 * it is easy to use tbz instruction to check whether loop tailing
> > +	 * count is less than 33 bytes, so as to bypass 2 unnecessary stps.
> > +	 */
> 
> do not use GNU style.  This one is GNU style:

This was copied exactly from memset_emag.S which has the style issue in it too. It was in 9627ab99b50 commit where this comment was introduced which copied from memset_base64.S.
Should we fix the other files or just the new files? Because it seems like having one version being based on the other once and then changing the style in one case but not the other seems wrong.
I suspect there many more GNU comment style issues in the aarch64 mem* functions even.

Thanks,
Andrew Pinski

> 
> > +	/* Set 64..96 bytes.  Write 64 bytes from the start and
> > +	   32 bytes from the end.  */
> 
> No separate start end end lines, no * indentation, space after the final .
> 
> Thanks,
> Florian


  reply	other threads:[~2024-05-14  7:40 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-05-08 16:59 [PATCH v2 1/2] Aarch64: Add memcpy for qualcomm's oryon-1 core Andrew Pinski
2024-05-08 16:59 ` [PATCH v2 2/2] Aarch64: Add new memset for Qualcomm's 0ryon-1 core Andrew Pinski
2024-05-13 13:48   ` Florian Weimer
2024-05-14  6:19     ` Andrew Pinski (QUIC)
2024-05-14  7:37       ` Florian Weimer
2024-05-14  7:47         ` Andrew Pinski (QUIC)
2024-05-14  9:10           ` Florian Weimer
2024-05-14  7:32   ` Florian Weimer
2024-05-14  7:40     ` Andrew Pinski (QUIC) [this message]
2024-05-22 23:01       ` Andrew Pinski (QUIC)
2024-05-23 13:08         ` Adhemerval Zanella Netto
2024-05-14  7:35 ` [PATCH v2 1/2] Aarch64: Add memcpy for qualcomm's oryon-1 core Florian Weimer

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=DM6PR02MB4058B6C2EEF2C3AF743FB575B8E32@DM6PR02MB4058.namprd02.prod.outlook.com \
    --to=quic_apinski@quicinc.com \
    --cc=fweimer@redhat.com \
    --cc=libc-alpha@sourceware.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).