Re: [PATCH 6/7] riscv: Add support for strlen inline expansion

[Jeff Law <jeffreyalaw@gmail.com>]

On 11/13/22 16:05, Christoph Muellner wrote:
> From: Christoph Müllner <christoph.muellner@vrull.eu>
>
> This patch implements the expansion of the strlen builtin
> using Zbb instructions (if available) for aligned strings
> using the following sequence:
>
>        li      a3,-1
>        addi    a4,a0,8
> .L2:  ld      a5,0(a0)
>        addi    a0,a0,8
>        orc.b   a5,a5
>        beq     a5,a3,6 <.L2>
>        not     a5,a5
>        ctz     a5,a5
>        srli    a5,a5,0x3
>        add     a0,a0,a5
>        sub     a0,a0,a4
>
> This allows to inline calls to strlen(), with optimized code for
> determining the length of a string.
>
> gcc/ChangeLog:
>
> 	* config/riscv/riscv-protos.h (riscv_expand_strlen): New
> 	  prototype.
> 	* config/riscv/riscv-string.cc (riscv_emit_unlikely_jump): New
> 	  function.
> 	(GEN_EMIT_HELPER2): New helper macro.
> 	(GEN_EMIT_HELPER3): New helper macro.
> 	(do_load_from_addr): New helper function.
> 	(riscv_expand_strlen_zbb): New function.
> 	(riscv_expand_strlen): New function.
> 	* config/riscv/riscv.md (strlen<mode>): Invoke expansion
> 	  functions for strlen.
>
>
> +extern bool riscv_expand_strlen (rtx[]);

Consider adding the number of elements in the RTX array here. Martin S's 
work from a little while ago will make use of it to try and catch 
over-reads and over-writes if the data is available.


>   
>   /* Information about one CPU we know about.  */
>   struct riscv_cpu_info {
> diff --git a/gcc/config/riscv/riscv-string.cc b/gcc/config/riscv/riscv-string.cc
> index 1137df475be..bf96522b608 100644
> --- a/gcc/config/riscv/riscv-string.cc
> +++ b/gcc/config/riscv/riscv-string.cc
> @@ -38,6 +38,81 @@
>   #include "predict.h"
>   #include "optabs.h"
>   
> +/* Emit unlikely jump instruction.  */
> +
> +static rtx_insn *
> +riscv_emit_unlikely_jump (rtx insn)
> +{
> +  rtx_insn *jump = emit_jump_insn (insn);
> +  add_reg_br_prob_note (jump, profile_probability::very_unlikely ());
> +  return jump;
> +}

I was a bit surprised that we didn't have this as a generic routine.   
Consider adding this to emit-rtl.cc along with its companion 
emit_likely_jump.  Not a requirement to move forward, but it seems like 
the right thing to do.




> +
> +/* Emit proper instruction depending on type of dest.  */

s/type/mode/



> +
> +/* Emit proper instruction depending on type of dest.  */

s/type/mode/


You probably want to undefine GEN_EMIT_HELPER once you're done when 
them.  That's become fairly standard practice for these kind of helper 
macros.

OK with the nits fixed.  Your call on whether or not to move the 
implementation of emit_likely_jump and emit_unlikely_jump into emit-rtl.cc.


Jeff