From: Richard Sandiford <richard.sandiford@arm.com>
To: Andrew Pinski <pinskia@gmail.com>
Cc: gcc-patches@gcc.gnu.org
Subject: Re: [PATCH] aarch64: [PR110986] Emit csinv again for `a ? ~b : b`
Date: Fri, 20 Oct 2023 13:13:26 +0100 [thread overview]
Message-ID: <mpta5sdlcex.fsf@arm.com> (raw)
In-Reply-To: <20231019040519.2655598-1-pinskia@gmail.com> (Andrew Pinski's message of "Wed, 18 Oct 2023 21:05:19 -0700")
Andrew Pinski <pinskia@gmail.com> writes:
> After r14-3110-g7fb65f10285, the canonical form for
> `a ? ~b : b` changed to be `-(a) ^ b` that means
> for aarch64 we need to add a few new insn patterns
> to be able to catch this and change it to be
> what is the canonical form for the aarch64 backend.
> A secondary pattern was needed to support a zero_extended
> form too; this adds a testcase for all 3 cases.
From the comment in the patch, it sounds like we don't really have
a target-independent canonical form. That is, we can't just rewrite
the old pattern to use the new form.
It would be nice there was a canonical form, but I won't push it.
> Bootstrapped and tested on aarch64-linux-gnu with no regressions.
>
> PR target/110986
>
> gcc/ChangeLog:
>
> * config/aarch64/aarch64.md (*cmov<mode>_insn_insv): New pattern.
> (*cmov_uxtw_insn_insv): Likewise.
>
> gcc/testsuite/ChangeLog:
>
> * gcc.target/aarch64/cond_op-1.c: New test.
> ---
> gcc/config/aarch64/aarch64.md | 46 ++++++++++++++++++++
> gcc/testsuite/gcc.target/aarch64/cond_op-1.c | 20 +++++++++
> 2 files changed, 66 insertions(+)
> create mode 100644 gcc/testsuite/gcc.target/aarch64/cond_op-1.c
>
> diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md
> index 32c7adc8928..59cd0415937 100644
> --- a/gcc/config/aarch64/aarch64.md
> +++ b/gcc/config/aarch64/aarch64.md
> @@ -4413,6 +4413,52 @@ (define_insn "*csinv3_uxtw_insn3"
> [(set_attr "type" "csel")]
> )
>
> +;; There are two canonical forms for `cmp ? ~a : a`.
> +;; This is the second form and is here to help combine.
> +;; Support `-(cmp) ^ a` into `cmp ? ~a : a`
> +;; The second pattern is to support the zero extend'ed version.
> +
> +(define_insn_and_split "*cmov<mode>_insn_insv"
> + [(set (match_operand:GPI 0 "register_operand" "=r")
> + (xor:GPI
> + (neg:GPI
> + (match_operator:GPI 1 "aarch64_comparison_operator"
> + [(match_operand 2 "cc_register" "") (const_int 0)]))
> + (match_operand:GPI 3 "general_operand" "r")))]
> + "can_create_pseudo_p ()"
> + "#"
> + "&& true"
IMO this is an ICE trap, since it hard-codes the assumption that there
will be a split pass after the last pre-LRA call to recog. I think we
should jsut provide the asm directly instead.
Looks good otherwise, thanks.
Richard
> + [(set (match_dup 0)
> + (if_then_else:GPI (match_dup 1)
> + (not:GPI (match_dup 3))
> + (match_dup 3)))]
> + {
> + operands[3] = force_reg (<MODE>mode, operands[3]);
> + }
> + [(set_attr "type" "csel")]
> +)
> +
> +(define_insn_and_split "*cmov_uxtw_insn_insv"
> + [(set (match_operand:DI 0 "register_operand" "=r")
> + (zero_extend:DI
> + (xor:SI
> + (neg:SI
> + (match_operator:SI 1 "aarch64_comparison_operator"
> + [(match_operand 2 "cc_register" "") (const_int 0)]))
> + (match_operand:SI 3 "general_operand" "r"))))]
> + "can_create_pseudo_p ()"
> + "#"
> + "&& true"
> + [(set (match_dup 0)
> + (if_then_else:DI (match_dup 1)
> + (zero_extend:DI (not:SI (match_dup 3)))
> + (zero_extend:DI (match_dup 3))))]
> + {
> + operands[3] = force_reg (SImode, operands[3]);
> + }
> + [(set_attr "type" "csel")]
> +)
> +
> ;; If X can be loaded by a single CNT[BHWD] instruction,
> ;;
> ;; A = UMAX (B, X)
> diff --git a/gcc/testsuite/gcc.target/aarch64/cond_op-1.c b/gcc/testsuite/gcc.target/aarch64/cond_op-1.c
> new file mode 100644
> index 00000000000..e6c7821127e
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/aarch64/cond_op-1.c
> @@ -0,0 +1,20 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2" } */
> +/* PR target/110986 */
> +
> +
> +long long full(unsigned a, unsigned b)
> +{
> + return a ? ~b : b;
> +}
> +unsigned fuu(unsigned a, unsigned b)
> +{
> + return a ? ~b : b;
> +}
> +long long fllll(unsigned long long a, unsigned long long b)
> +{
> + return a ? ~b : b;
> +}
> +
> +/* { dg-final { scan-assembler-times "csinv\tw\[0-9\]*" 2 } } */
> +/* { dg-final { scan-assembler-times "csinv\tx\[0-9\]*" 1 } } */
next prev parent reply other threads:[~2023-10-20 12:13 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-10-19 4:05 Andrew Pinski
2023-10-20 12:13 ` Richard Sandiford [this message]
2023-10-20 13:17 ` Richard Earnshaw
2023-10-20 13:42 ` Richard Sandiford
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=mpta5sdlcex.fsf@arm.com \
--to=richard.sandiford@arm.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=pinskia@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).