Re: [PATCH 2/3 v2] xtensa: Add 'adddi3' and 'subdi3' insn patterns

[Max Filippov <jcmvbkbc@gmail.com>]

On Tue, May 30, 2023 at 2:50 AM Takayuki 'January June' Suwa
<jjsuwa_sys3175@yahoo.co.jp> wrote:
>
> Resubmitting the correct one due to a mistake in merging order of fixes.
> ---
> More optimized than the default RTL generation.
>
> gcc/ChangeLog:
>
>         * config/xtensa/xtensa.md (adddi3, subdi3):
>         New RTL generation patterns implemented according to the instruc-
>         tion idioms described in the Xtensa ISA reference manual (p. 600).
> ---
>  gcc/config/xtensa/xtensa.md | 52 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 52 insertions(+)
>
> diff --git a/gcc/config/xtensa/xtensa.md b/gcc/config/xtensa/xtensa.md
> index eda1353894b..6882baaedfd 100644
> --- a/gcc/config/xtensa/xtensa.md
> +++ b/gcc/config/xtensa/xtensa.md
> @@ -190,6 +190,32 @@
>     (set_attr "mode"    "SI")
>     (set_attr "length"  "3")])
>
> +(define_expand "adddi3"
> +  [(set (match_operand:DI 0 "register_operand")
> +       (plus:DI (match_operand:DI 1 "register_operand")
> +                (match_operand:DI 2 "register_operand")))]
> +  ""
> +{
> +  rtx lo_dest, hi_dest, lo_op0, hi_op0, lo_op1, hi_op1;
> +  rtx_code_label *label;
> +  lo_dest = gen_lowpart (SImode, operands[0]);
> +  hi_dest = gen_highpart (SImode, operands[0]);
> +  lo_op0 = gen_lowpart (SImode, operands[1]);
> +  hi_op0 = gen_highpart (SImode, operands[1]);
> +  lo_op1 = gen_lowpart (SImode, operands[2]);
> +  hi_op1 = gen_highpart (SImode, operands[2]);
> +  if (rtx_equal_p (lo_dest, lo_op1))
> +    FAIL;

With this condition I see the following source

unsigned long long foo(unsigned long long a, unsigned long long b)
{
       return a + b;
}

turns to (expected)

       .global foo
       .type   foo, @function
foo:
       add.n   a2, a2, a4
       add.n   a3, a3, a5
       bgeu    a2, a4, .L2
       addi.n  a3, a3, 1
.L2:
       ret.n

but

unsigned long long foo(unsigned long long a, unsigned long long b)
{
       return b + a;
}

has an extra instruction:

       .global foo
       .type   foo, @function
foo:
       mov.n   a9, a2
       add.n   a2, a4, a2
       add.n   a3, a5, a3
       bgeu    a2, a9, .L2
       addi.n  a3, a3, 1
.L2:
       ret.n

I though that maybe the following would help (plus using
lo_cmp in the emit_cmp_and_jump_insns below):

  if (!rtx_equal_p (lo_dest, lo_op0))
   lo_cmp = lo_op0;
 else if (!rtx_equal_p (lo_dest, lo_op1))
   lo_cmp = lo_op1;
 else
   FAIL;

but to my surprise it doesn't.

> +  emit_clobber (operands[0]);

Why is this clobber needed?

> +  emit_insn (gen_addsi3 (lo_dest, lo_op0, lo_op1));
> +  emit_insn (gen_addsi3 (hi_dest, hi_op0, hi_op1));
> +  emit_cmp_and_jump_insns (lo_dest, lo_op1, GEU, const0_rtx,
> +                          SImode, true, label = gen_label_rtx ());
> +  emit_insn (gen_addsi3 (hi_dest, hi_dest, const1_rtx));
> +  emit_label (label);
> +  DONE;
> +})
> +
>  (define_insn "addsf3"
>    [(set (match_operand:SF 0 "register_operand" "=f")
>         (plus:SF (match_operand:SF 1 "register_operand" "%f")
> @@ -237,6 +263,32 @@
>                       (const_int 5)
>                       (const_int 6)))])
>
> +(define_expand "subdi3"
> +  [(set (match_operand:DI 0 "register_operand")
> +       (minus:DI (match_operand:DI 1 "register_operand")
> +                 (match_operand:DI 2 "register_operand")))]
> +  ""
> +{
> +  rtx lo_dest, hi_dest, lo_op0, hi_op0, lo_op1, hi_op1;
> +  rtx_code_label *label;
> +  lo_dest = gen_lowpart (SImode, operands[0]);
> +  hi_dest = gen_highpart (SImode, operands[0]);
> +  lo_op0 = gen_lowpart (SImode, operands[1]);
> +  hi_op0 = gen_highpart (SImode, operands[1]);
> +  lo_op1 = gen_lowpart (SImode, operands[2]);
> +  hi_op1 = gen_highpart (SImode, operands[2]);
> +  if (rtx_equal_p (lo_op0, lo_op1))
> +    FAIL;

I believe that for the emit_cmp_and_jump_insns below
the check here should look like this:

if (rtx_equal_p (lo_dest, lo_op0) || rtx_equal_p (lo_dest, lo_op1))

But maybe drop this check and use the following instead?

 emit_insn (gen_subsi3 (hi_dest, hi_op0, hi_op1));
 emit_cmp_and_jump_insns (lo_op0, lo_op1, GEU, const0_rtx,
                          SImode, true, label = gen_label_rtx ());
 emit_insn (gen_addsi3 (hi_dest, hi_dest, constm1_rtx));
 emit_label (label);
 emit_insn (gen_subsi3 (lo_dest, lo_op0, lo_op1));

> +  emit_clobber (operands[0]);

Why is this clobber needed?

> +  emit_insn (gen_subsi3 (lo_dest, lo_op0, lo_op1));
> +  emit_insn (gen_subsi3 (hi_dest, hi_op0, hi_op1));
> +  emit_cmp_and_jump_insns (lo_op0, lo_op1, GEU, const0_rtx,
> +                          SImode, true, label = gen_label_rtx ());
> +  emit_insn (gen_addsi3 (hi_dest, hi_dest, constm1_rtx));
> +  emit_label (label);
> +  DONE;
> +})
> +
>  (define_insn "subsf3"
>    [(set (match_operand:SF 0 "register_operand" "=f")
>         (minus:SF (match_operand:SF 1 "register_operand" "f")
> --
> 2.30.2

-- 
Thanks.
-- Max