Received-SPF: pass (google.com: domain of linux-crypto+bounces-3418-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <4D8C090A8BD54C05A97F57A0F640E94F@H270>
From: "Stefan Kanthak" <stefan.kanthak@nexgo.de>
To: "Eric Biggers" <ebiggers@kernel.org>,
	<ardb@kernel.org>
Cc: <linux-crypto@vger.kernel.org>
References: <5EEE09A9021540A5AAD8BFEEE915512D@H270> <20240408123734.GB732@quark.localdomain> <9088939CC5454139901CEDD97DAFB004@H270> <20240408151832.GE732@quark.localdomain>
In-Reply-To: <20240408151832.GE732@quark.localdomain>
Subject: Re: [PATCH 1/2] crypto: s(h)aving 40+ bytes off arch/x86/crypto/sha256_ni_asm.S
Date: Tue, 9 Apr 2024 12:23:13 +0200
Organization: Me, myself & IT
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain;
	charset="iso-8859-1"
Content-Transfer-Encoding: 7bit

"Eric Biggers" <ebiggers@kernel.org> wrote:

> [+Cc linux-crypto]
> 
> Please use reply-all so that the list gets included.
> 
> On Mon, Apr 08, 2024 at 04:15:32PM +0200, Stefan Kanthak wrote:
>> Hi Eric,
>> 
>> > On Mon, Apr 08, 2024 at 11:26:52AM +0200, Stefan Kanthak wrote:
>> >> Use shorter SSE2 instructions instead of some SSE4.1
>> >> use short displacements into K256
>> >> 
>> >> --- -/arch/x86/crypto/sha256_ni_asm.S
>> >> +++ +/arch/x86/crypto/sha256_ni_asm.S
>> > 
>> > Thanks!  I'd like to benchmark this to see how it affects performance,
>> 
>> Performance is NOT affected: if CPUs weren't superscalar, the patch might
>> save 2 to 4 processor cycles as it replaces palignr/pblendw (slow) with
>> punpck*qdq (fast and shorter)
>> 
>> > but unfortunately this patch doesn't apply.  It looks your email client
>> > corrupted your patch by replacing tabs with spaces.  Can you please use
>> > 'git send-email' to send patches?
>> 
>> I don't use git at all; I'll use cURL instead.

[...]

>> > Please make sure to run the crypto self-tests too.
>> 
>> I can't, I don't use Linux at all; I just noticed that this function uses
>> 4-byte displacements and palignr/pblendw instead of punpck?qdq after pshufd 
>> 
>> > The above is storing the two halves of the state in the wrong order.
>> 
>> ARGH, you are right; I recognized it too, wanted to correct it, but was
>> interrupted and forgot it after returning to the patch. Sorry.
> 
> I'm afraid that if you don't submit a probably formatted and tested patch, your
> patch can't be accepted.  We can treat it as a suggestion, though since you're
> sending actual code it would really help if it had your Signed-off-by.

Treat is as suggestion.
I but wonder that in the past 9 years since Tim Chen submitted the SHA-NI code
(which was copied umpteen times by others and included in their own code bases)
nobody noticed/questioned (or if so, bothered to submit a patch like mine, that
reduces the code size by 5%, upstream) why he used 16x "pshufd $14, %xmm0, %xmm0"
instead of the 1 byte shorter "punpckhqdq %xmm0, %xmm0" or "psrldq $8, %xmm0"
(which both MAY execute on more ports or faster than the shuffle instructions,
depending on the micro-architecture), why he used 8x a 4-byte instead of a 1-byte
displacement, or why he used "palignr/pblendw" instead of the shorter "punpck?qdq".

regards
Stefan

PS: aaaahhhh, you picked my suggestion up and applied it to the AES routine.