Re: [PATCH] widening_mul, i386: Improve spaceship expansion on x86 [PR103973]

[Uros Bizjak <ubizjak@gmail.com>]

On Fri, Jan 14, 2022 at 11:56 PM Jakub Jelinek <jakub@redhat.com> wrote:
>
> Hi!
>
> C++20:
> #include <compare>
> auto cmp4way(double a, double b)
> {
>   return a <=> b;
> }
> expands to:
>         ucomisd %xmm1, %xmm0
>         jp      .L8
>         movl    $0, %eax
>         jne     .L8
> .L2:
>         ret
>         .p2align 4,,10
>         .p2align 3
> .L8:
>         comisd  %xmm0, %xmm1
>         movl    $-1, %eax
>         ja      .L2
>         ucomisd %xmm1, %xmm0
>         setbe   %al
>         addl    $1, %eax
>         ret
> That is 3 comparisons of the same operands.
> The following patch improves it to just one comparison:
>         comisd  %xmm1, %xmm0
>         jp      .L4
>         seta    %al
>         movl    $0, %edx
>         leal    -1(%rax,%rax), %eax
>         cmove   %edx, %eax
>         ret
> .L4:
>         movl    $2, %eax
>         ret
> While a <=> b expands to a == b ? 0 : a < b ? -1 : a > b ? 1 : 2
> where the first comparison is equality and this shouldn't raise
> exceptions on qNaN operands, if the operands aren't equal (which
> includes unordered cases), then it immediately performs < or >
> comparison and that raises exceptions even on qNaNs, so we can just
> perform a single comparison that raises exceptions on qNaN.
> As the 4 different cases are encoded as
> ZF CF PF
> 1  1  1  a unordered b
> 0  0  0  a > b
> 0  1  0  a < b
> 1  0  0  a == b
> we can emit optimal sequence of comparions, first jp
> for the unordered case, then je for the == case and finally jb
> for the < case.
>
> The patch pattern recognizes spaceship-like comparisons during
> widening_mul if the spaceship optab is implemented, and replaces
> thoose comparisons with comparisons of .SPACESHIP ifn which returns
> -1/0/1/2 based on the comparison.  This seems to work well both for the
> case of just returning the -1/0/1/2 (when we have just a common
> successor with a PHI) or when the different cases are handled with
> various other basic blocks.  The testcases cover both of those cases,
> the latter with different function calls in those.
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
>
> 2022-01-14  Jakub Jelinek  <jakub@redhat.com>
>
>         PR target/103973
>         * tree-cfg.h (cond_only_block_p): Declare.
>         * tree-ssa-phiopt.c (cond_only_block_p): Move function to ...
>         * tree-cfg.c (cond_only_block_p): ... here.  No longer static.
>         * optabs.def (spaceship_optab): New optab.
>         * internal-fn.def (SPACESHIP): New internal function.
>         * internal-fn.h (expand_SPACESHIP): Declare.
>         * internal-fn.c (expand_PHI): Formatting fix.
>         (expand_SPACESHIP): New function.
>         * tree-ssa-math-opts.c (optimize_spaceship): New function.
>         (math_opts_dom_walker::after_dom_children): Use it.
>         * config/i386/i386.md (spaceship<mode>3): New define_expand.
>         * config/i386/i386-protos.h (ix86_expand_fp_spaceship): Declare.
>         * config/i386/i386-expand.c (ix86_expand_fp_spaceship): New function.
>         * doc/md.texi (spaceship@var{m}3): Document.
>
>         * gcc.target/i386/pr103973-1.c: New test.
>         * gcc.target/i386/pr103973-2.c: New test.
>         * gcc.target/i386/pr103973-3.c: New test.
>         * gcc.target/i386/pr103973-4.c: New test.
>         * g++.target/i386/pr103973-1.C: New test.
>         * g++.target/i386/pr103973-2.C: New test.
>         * g++.target/i386/pr103973-3.C: New test.
>         * g++.target/i386/pr103973-4.C: New test.

>--- gcc/config/i386/i386.md.jj  2022-01-14 11:51:34.432384170 +0100
> +++ gcc/config/i386/i386.md     2022-01-14 18:22:41.140906449 +0100
> @@ -23886,6 +23886,18 @@ (define_insn "hreset"
>    [(set_attr "type" "other")
>     (set_attr "length" "4")])
>
> +;; Spaceship optimization
> +(define_expand "spaceship<mode>3"
> +  [(match_operand:SI 0 "register_operand")
> +   (match_operand:MODEF 1 "cmp_fp_expander_operand")
> +   (match_operand:MODEF 2 "cmp_fp_expander_operand")]
> +  "(TARGET_80387 || (SSE_FLOAT_MODE_P (<MODE>mode) && TARGET_SSE_MATH))
> +   && TARGET_CMOVE && TARGET_IEEE_FP"

Is there a reason that this pattern is limited to TARGET_IEEE_FP?
During the expansion in ix86_expand_fp_spaceship, we can just skip
jump on unordered, while ix86_expand_fp_compare will emit the correct
comparison mode depending on TARGET_IEEE_FP.

> +{
> +  ix86_expand_fp_spaceship (operands[0], operands[1], operands[2]);
> +  DONE;
> +})
> +
>  (include "mmx.md")
>  (include "sse.md")
>  (include "sync.md")
> --- gcc/config/i386/i386-expand.c.jj    2022-01-14 11:51:34.429384213 +0100
> +++ gcc/config/i386/i386-expand.c       2022-01-14 21:02:09.446437810 +0100
> @@ -2879,6 +2879,46 @@ ix86_expand_setcc (rtx dest, enum rtx_co
>    emit_insn (gen_rtx_SET (dest, ret));
>  }
>
> +/* Expand floating point op0 <=> op1 if NaNs are honored.  */
> +
> +void
> +ix86_expand_fp_spaceship (rtx dest, rtx op0, rtx op1)
> +{
> +  gcc_checking_assert (ix86_fp_comparison_strategy (GT) == IX86_FPCMP_COMI);
> +  rtx gt = ix86_expand_fp_compare (GT, op0, op1);
> +  rtx l0 = gen_label_rtx ();
> +  rtx l1 = gen_label_rtx ();
> +  rtx l2 = gen_label_rtx ();
> +  rtx lend = gen_label_rtx ();
> +  rtx un = gen_rtx_fmt_ee (UNORDERED, VOIDmode,
> +                          gen_rtx_REG (CCFPmode, FLAGS_REG), const0_rtx);
> +  rtx tmp = gen_rtx_IF_THEN_ELSE (VOIDmode, un,
> +                                 gen_rtx_LABEL_REF (VOIDmode, l2), pc_rtx);
> +  rtx_insn *jmp = emit_jump_insn (gen_rtx_SET (pc_rtx, tmp));
> +  add_reg_br_prob_note (jmp, profile_probability:: very_unlikely ());

Please also add JUMP_LABEL to the insn.

> +  rtx eq = gen_rtx_fmt_ee (UNEQ, VOIDmode,
> +                          gen_rtx_REG (CCFPmode, FLAGS_REG), const0_rtx);
> +  tmp = gen_rtx_IF_THEN_ELSE (VOIDmode, eq,
> +                             gen_rtx_LABEL_REF (VOIDmode, l0), pc_rtx);
> +  jmp = emit_jump_insn (gen_rtx_SET (pc_rtx, tmp));
> +  add_reg_br_prob_note (jmp, profile_probability::unlikely ());
> +  tmp = gen_rtx_IF_THEN_ELSE (VOIDmode, gt,
> +                             gen_rtx_LABEL_REF (VOIDmode, l1), pc_rtx);
> +  jmp = emit_jump_insn (gen_rtx_SET (pc_rtx, tmp));
> +  add_reg_br_prob_note (jmp, profile_probability::even ());
> +  emit_move_insn (dest, constm1_rtx);
> +  emit_jump (lend);
> +  emit_label (l0);

and LABEL_NUSES label.

> +  emit_move_insn (dest, const0_rtx);
> +  emit_jump (lend);
> +  emit_label (l1);
> +  emit_move_insn (dest, const1_rtx);
> +  emit_jump (lend);
> +  emit_label (l2);
> +  emit_move_insn (dest, const2_rtx);
> +  emit_label (lend);
> +}
> +
>  /* Expand comparison setting or clearing carry flag.  Return true when
>     successful and set pop for the operation.  */
>  static bool

Uros.