Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 11 May 2023 09:43:26 +0200
From:   Andrew Jones <ajones@ventanamicro.com>
To:     zhangfei <zhang_fei_0403@163.com>
Cc:     aou@eecs.berkeley.edu, conor.dooley@microchip.com,
        linux-kernel@vger.kernel.org, linux-riscv@lists.infradead.org,
        palmer@dabbelt.com, paul.walmsley@sifive.com,
        zhangfei@nj.iscas.ac.cn
Subject: Re: [PATCH v2 2/2] RISC-V: lib: Optimize memset performance
Message-ID: <20230511-0b91da227b91eee76f98c6b0@orel>
References: <20230511012604.3222-1-zhang_fei_0403@163.com>
 <20230511013453.3275-1-zhang_fei_0403@163.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230511013453.3275-1-zhang_fei_0403@163.com>
Precedence: bulk

On Thu, May 11, 2023 at 09:34:53AM +0800, zhangfei wrote:
> From: zhangfei <zhangfei@nj.iscas.ac.cn>
> 
> Optimized performance when the data size is less than 16 bytes.
> Compared to byte by byte storage, significant performance improvement has been achieved.
> It allows storage instructions to be executed in parallel and reduces the number of jumps.

Please wrap commit message lines at 74 chars.

> Additional checks can avoid redundant stores.
> 
> Signed-off-by: Fei Zhang <zhangfei@nj.iscas.ac.cn>
> ---
>  arch/riscv/lib/memset.S | 40 +++++++++++++++++++++++++++++++++++++---
>  1 file changed, 37 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/riscv/lib/memset.S b/arch/riscv/lib/memset.S
> index e613c5c27998..452764bc9900 100644
> --- a/arch/riscv/lib/memset.S
> +++ b/arch/riscv/lib/memset.S
> @@ -106,9 +106,43 @@ WEAK(memset)
>  	beqz	a2, 6f
>  	add	a3, t0, a2
>  5:
> -	sb	a1, 0(t0)
> -	addi	t0, t0, 1
> -	bltu	t0, a3, 5b
> +       /* fill head and tail with minimal branching */
> +       sb      a1,  0(t0)
> +       sb      a1, -1(a3)
> +       li 	a4, 2
> +       bgeu 	a4, a2, 6f
> +
> +       sb 	a1,  1(t0)
> +       sb 	a1,  2(t0)
> +       sb 	a1, -2(a3)
> +       sb 	a1, -3(a3)
> +       li 	a4, 6
> +       bgeu 	a4, a2, 6f
> +
> +       /* 
> +        * Adding additional detection to avoid 
> +        * redundant stores can lead 
> +        * to better performance
> +        */
> +       sb 	a1,  3(t0)
> +       sb 	a1, -4(a3)
> +       li 	a4, 8
> +       bgeu 	a4, a2, 6f
> +
> +       sb 	a1,  4(t0)
> +       sb 	a1, -5(a3)
> +       li 	a4, 10
> +       bgeu 	a4, a2, 6f

These extra checks feel ad hoc to me. Naturally you'll get better results
for 8 byte memsets when there's a branch to the ret after 8 bytes. But
what about 9? I'd think you'd want benchmarks from 1 to 15 bytes to show
how it performs better or worse than byte by byte for each of those sizes.
Also, while 8 bytes might be worth special casing, I'm not sure why 10
would be. What makes 10 worth optimizing more than 11?

Finally, microbenchmarking is quite hardware-specific and energy
consumption should probably also be considered. What energy cost is
there from making redundant stores? Is it worth it?

Thanks for cleaning up the patch series, but I'm still not 100%
convinced we want it.

> +
> +       sb 	a1,  5(t0)
> +       sb 	a1,  6(t0)
> +       sb 	a1, -6(a3)
> +       sb 	a1, -7(a3)
> +       li 	a4, 14
> +       bgeu 	a4, a2, 6f
> +       
> +       /* store the last byte */
> +       sb 	a1,  7(t0)
>  6:
>  	ret
>  END(__memset)
> -- 
> 2.33.0
>

Thanks,
drew