DPDK patches and discussions
 help / color / mirror / Atom feed
From: Thomas Monjalon <thomas@monjalon.net>
To: Shreesh Adiga <16567adigashreesh@gmail.com>
Cc: Bruce Richardson <bruce.richardson@intel.com>,
	Konstantin Ananyev <konstantin.v.ananyev@yandex.ru>,
	Jasvinder Singh <jasvinder.singh@intel.com>,
	dev@dpdk.org
Subject: Re: [PATCH] net/crc: reduce usage of static arrays in net_crc_sse.c
Date: Wed, 01 Oct 2025 14:16:15 +0200	[thread overview]
Message-ID: <22593368.0c2gjJ1VT2@thomas> (raw)
In-Reply-To: <CA+-x59aZzXLWACV17OaHCyUU_WnqfdGdJqjJqMeHmLX49G_2mg@mail.gmail.com>

01/10/2025 12:24, Shreesh Adiga:
> On Wed, Oct 1, 2025 at 1:25 PM Thomas Monjalon <thomas@monjalon.net> wrote:
> 
> > 29/09/2025 18:28, Shreesh Adiga:
> > > On Wed, Sep 24, 2025 at 8:28 PM Thomas Monjalon <thomas@monjalon.net>
> > wrote:
> > >
> > > > Hello,
> > > >
> > > > 16/07/2025 12:34, Shreesh Adiga:
> > > > > Replace the clearing of lower 32 bits of XMM register with blend of
> > > > > zero register.
> > > > > Replace the clearing of upper 64 bits of XMM register with
> > > > _mm_move_epi64.
> > > > > Clang is able to optimize away the AND + memory operand with the
> > > > > above sequence, however GCC is still emitting the code for AND with
> > > > > memory operands which is being explicitly eliminated here.
> > > > >
> > > > > Additionally replace the 48 byte crc_xmm_shift_tab with the contents
> > of
> > > > > shf_table which is 32 bytes, achieving the same functionality.
> > > > >
> > > > > Signed-off-by: Shreesh Adiga <16567adigashreesh@gmail.com>
> > > >
> > > > Sorry I'm not following.
> > > > Please could you start with defining the goal of this patch?
> > > > Is it a code simplification or a performance optimization?
> > >
> > > It is intended to be a minor performance optimization.
> >
> > Please could you give some performance numbers in the commit log?
> >
> I don't think that this change can be reliably measured. The changes only
> impact
> the last stage crc 64 to 32 fold and the last 16 bytes computation. The
> impact will only
> be a couple of clock cycles at best. Reducing the static array usage also I
> don't know
> if it can be reliably measured especially since it is not affecting the
> main loop.
> This patch can be ignored if minor incremental changes are not desirable.

Minor changes are desirable.
I'm just asking to understand the real impact of the change.
In general when doing an optimization we try to test it
and give some numbers.
Note we have a unit test for CRC in app/test/test_crc.c
but no performance test for it.

I'll wait for a review from an x86 maintainer.



      reply	other threads:[~2025-10-01 12:16 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2025-07-16 10:34 Shreesh Adiga
2025-09-24 14:58 ` Thomas Monjalon
2025-09-29 16:28   ` Shreesh Adiga
2025-10-01  7:55     ` Thomas Monjalon
2025-10-01 10:24       ` Shreesh Adiga
2025-10-01 12:16         ` Thomas Monjalon [this message]

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=22593368.0c2gjJ1VT2@thomas \
    --to=thomas@monjalon.net \
    --cc=16567adigashreesh@gmail.com \
    --cc=bruce.richardson@intel.com \
    --cc=dev@dpdk.org \
    --cc=jasvinder.singh@intel.com \
    --cc=konstantin.v.ananyev@yandex.ru \
    /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 NNTP newsgroup(s).