External email: Use caution opening links or attachments
On 05/11/2025 10:39, Dhruv Chawla wrote:
On 28/10/25 22:03, Alex Coplan wrote:
External email: Use caution opening links or attachments
Hi Dhruv,
Sorry for the long wait on this one. Comments below ...
Hi Alex,
Thanks for the review. I have attached a new version of the patch
to this email.
Thanks, and sorry to be slow again...
On 18/08/2025 21:31, [email protected] wrote:
From: Dhruv Chawla <[email protected]>
This patch folds the following patterns:
- umax (a, add (a, b)) -> [sum, ovf] = adds (a, b); !ovf ? sum : a
- umin (a, add (a, b)) -> [sum, ovf] = adds (a, b); !ovf ? a : sum
- umax (a, sub (a, b)) -> [diff, ovf] = subs (a, b); ovf ? diff : a
- umin (a, sub (a, b)) -> [diff, ovf] = subs (a, b); ovf ? a : diff
Where ovf is the overflow flag. adds and subs are generated by
generating a parallel compare+plus/minus which maps to the pattern
add<mode>3_compareC. sub<mode>3_compareC is also created to have an
equivalent pattern for the subs instruction. In the case of subs, the
overflow flag represents underflow.
This patch is a respin of the patch posted at
https://gcc.gnu.org/pipermail/gcc-patches/2025-May/685021.html as per
the suggestion to turn it into a target-specific transform by Richard
Biener.
Bootstrapped and regtested on aarch64-unknown-linux-gnu.
Signed-off-by: Dhruv Chawla <[email protected]>
PR middle-end/116815
gcc/ChangeLog:
* config/aarch64/aarch64.md (sub<mode>3_compareC): New pattern.
(*aarch64_plus_within_<optab><mode>3_<ovf_commutate>): Likewise.
(*aarch64_minus_within_<optab><mode>3): Likewise.
* config/aarch64/iterators.md (ovf_add_cmp): New code attribute.
(ovf_sub_cmp): Likewise.
(ovf_commutate): New iterator.
(ovf_comm_opp): New int attribute.
gcc/testsuite/ChangeLog:
* gcc.target/aarch64/pr116815-1.c: New test.
* gcc.target/aarch64/pr116815-2.c: Likewise.
* gcc.target/aarch64/pr116815-3.c: Likewise.
* gcc.target/aarch64/pr116815-4.c: Likewise.
* gcc.target/aarch64/pr116815-5.c: Likewise.
* gcc.target/aarch64/pr116815-6.c: Likewise.
---
gcc/config/aarch64/aarch64.md | 76 +++++++++++
gcc/config/aarch64/iterators.md | 9 ++
gcc/testsuite/gcc.target/aarch64/pr116815-1.c | 120 ++++++++++++++++++
gcc/testsuite/gcc.target/aarch64/pr116815-2.c | 93 ++++++++++++++
gcc/testsuite/gcc.target/aarch64/pr116815-3.c | 62 +++++++++
gcc/testsuite/gcc.target/aarch64/pr116815-4.c | 48 +++++++
gcc/testsuite/gcc.target/aarch64/pr116815-5.c | 44 +++++++
gcc/testsuite/gcc.target/aarch64/pr116815-6.c | 60 +++++++++
8 files changed, 512 insertions(+)
create mode 100644 gcc/testsuite/gcc.target/aarch64/pr116815-1.c
create mode 100644 gcc/testsuite/gcc.target/aarch64/pr116815-2.c
create mode 100644 gcc/testsuite/gcc.target/aarch64/pr116815-3.c
create mode 100644 gcc/testsuite/gcc.target/aarch64/pr116815-4.c
create mode 100644 gcc/testsuite/gcc.target/aarch64/pr116815-5.c
create mode 100644 gcc/testsuite/gcc.target/aarch64/pr116815-6.c
diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md
index 8e431a10cb1..a55fe5c85d8 100644
--- a/gcc/config/aarch64/aarch64.md
+++ b/gcc/config/aarch64/aarch64.md
@@ -3715,6 +3715,20 @@
[(set_attr "type" "alus_sreg")]
)
+;; An equivalent to add<mode>3_compareC
+(define_insn "sub<mode>3_compareC"
+ [(set (reg:CC_C CC_REGNUM)
+ (compare:CC_C
+ (minus:GPI
+ (match_operand:GPI 1 "register_operand" "r")
+ (match_operand:GPI 2 "register_operand" "r"))
+ (match_dup 1)))
+ (set (match_operand:GPI 0 "register_operand" "=r")
+ (minus:GPI (match_dup 1) (match_dup 2)))]
+ ""
+ "subs\t%<w>0, %<w>1, %<w>2"
+)
+
I don't think we want/need this pattern. subs(a,b) underflows precisely
when a < b, i.e. when a - b < 0 so you can just use a plain CCmode
comparison. I think you can make use of the existing
sub<mode>3_compare1 pattern here.
This should simplify the define_insn_and_split for the subs case below.
(define_peephole2
[(set (match_operand:GPI 0 "aarch64_general_reg")
(minus:GPI (match_operand:GPI 1 "aarch64_reg_or_zero")
@@ -4455,6 +4469,68 @@
[(set_attr "type" "<su>div")]
)
+;; umax (a, add (a, b)) => [sum, ovf] = adds (a, b); !ovf ? sum : a
+;; umin (a, add (a, b)) => [sum, ovf] = adds (a, b); !ovf ? a : sum
+;; ... along with the commutative version of add (a, b) i.e. add (b, a).
For the sake of saving one comment line, it might be better to actually
spell out the commutated variant, AIUI e.g. for umax that's:
umax (b, add (a, b)) => [sum, ovf] = adds (b, a); !ovf ? sum : b
Done. I also added this to the patch description.
+(define_insn_and_split "*aarch64_plus_within_<optab><mode>3_<ovf_commutate>"
+ [(set (match_operand:GPI 0 "register_operand" "=r")
+ (UMAXMIN:GPI
+ (plus:GPI (match_operand:GPI 1 "register_operand" "r")
+ (match_operand:GPI 2 "register_operand" "r"))
+ (match_dup ovf_commutate)))
+ (clobber (match_scratch:GPI 3))]
I think you might want an "=r" constraint on the match_scratch here.
I am a bit confused here - I expect this pattern to only execute pre-RA
because it requires being able to call gen_reg_rtx (which only works
pre-RA). Does RA populate scratch registers by itself? Otherwise, I
don't really see any other way for this split to run post-RA.
So having done some digging on this, the point of this idiom:
+ if (GET_CODE (operands[3]) == SCRATCH)
+ operands[3] = gen_reg_rtx (<MODE>mode);
is to allow the splitter to work both before and after RA. Indeed, the
RA will allocate hard registers in place of scratch registers, provided
there is a suitable constraint on the operand.
I think whatever we do the split condition should be "&& 1"; as Uros
pointed out, allowing the insn to match after RA but not allowing the
insn to be split is an ICE trap.
We then need to decide whether to restrict the define_insn_and_splits to
pre-RA only, or allow them both before and after RA. I don't see a good
reason to forbid them from matching after RA, so I think the patch is OK
with the constraints on the scratch registers added, and the split
conditions changed to "&& 1". I've added comments to this effect below.
Note that if we _were_ to restrict the pattern to pre-RA only, I agree
with all of Uros's comments, and we would have to define
aarch64_pre_reload_split and use that in the insn condition for the
define_insn_and_splits, but incrementally it seems easier to me to just
allow the pattern to work both before and after RA.
Uros: IIRC I ran into issues where unconditionally calling gen_reg_rtx
still caused an ICE, maybe because the code ran during reload itself.
I guess I could replace "!reload_completed" with "can_create_pseudo_p ()"?
+ "!TARGET_CSSC"
+ "#"
+ "&& !reload_completed"
+ [(parallel
+ [(set (reg:CC_C CC_REGNUM)
+ (compare:CC_C (plus:GPI (match_dup ovf_commutate)
+ (match_dup <ovf_comm_opp>))
+ (match_dup ovf_commutate)))
+ (set (match_dup 3) (plus:GPI (match_dup ovf_commutate)
+ (match_dup <ovf_comm_opp>)))])
+ (set (match_dup 0)
+ (if_then_else:GPI (<ovf_add_cmp> (reg:CC_C CC_REGNUM)
+ (const_int 0))
+ (match_dup 3)
+ (match_dup ovf_commutate)))]
+ {
+ if (GET_CODE (operands[3]) == SCRATCH)
+ operands[3] = gen_reg_rtx (<MODE>mode);
At first I was a bit unsure about this idiom, since md.texi:9358 has
this to say about define_split (and therefore about
define_insn_and_split):
The @var{preparation-statements} are similar to those statements that
are specified for @code{define_expand} (@pxref{Expander Definitions})
and are executed before the new RTL is generated to prepare for the
generated code or emit some insns whose pattern is not fixed. Unlike
those in @code{define_expand}, however, these statements must not
generate any new pseudo-registers. Once reload has completed, they also
must not allocate any space in the stack frame.
but I see that we already use this idiom for a few patterns in aarch64-sve.md.
Would appreciate a second opinion on this. Either the existing SVE patterns are
wrong or (perhaps more likely) the md.texi docs are out of date.
+ }
+)
+
+;; In this case the ovf represents an underflow.
+;; umax (a, sub (a, b)) => [diff, ovf] = subs (a, b); ovf ? diff : a
+;; umin (a, sub (a, b)) => [diff, ovf] = subs (a, b); ovf ? a : diff
+(define_insn_and_split "*aarch64_minus_within_<optab><mode>3"
+ [(set (match_operand:GPI 0 "register_operand" "=r")
+ (UMAXMIN:GPI
+ (minus:GPI (match_operand:GPI 1 "register_operand" "r")
+ (match_operand:GPI 2 "register_operand" "r"))
+ (match_dup 1)))
+ (clobber (match_scratch:GPI 3))]
+ "!TARGET_CSSC"
+ "#"
+ "&& !reload_completed"
+ [(parallel
+ [(set (reg:CC_C CC_REGNUM)
+ (compare:CC_C (minus:GPI (match_dup 1) (match_dup 2))
+ (match_dup 1)))
+ (set (match_dup 3) (minus:GPI (match_dup 1) (match_dup 2)))])
+ ;; There is an inverse mapping here from CC_C -> CC as this requires the
+ ;; inverse of the comparison from the above pattern.
As mentioned above you should be able to do the whole thing in CCmode, which
should simplify things here.
+ (set (match_dup 0)
+ (if_then_else:GPI (<ovf_sub_cmp> (reg:CC CC_REGNUM)
+ (const_int 0))
+ (match_dup 3)
+ (match_dup 1)))]
+ {
+ if (GET_CODE (operands[3]) == SCRATCH)
+ operands[3] = gen_reg_rtx (<MODE>mode);
+ }
+)
+
;; -------------------------------------------------------------------
;; Comparison insns
;; -------------------------------------------------------------------
diff --git a/gcc/config/aarch64/iterators.md b/gcc/config/aarch64/iterators.md
index c3771d9402b..a29243247bd 100644
--- a/gcc/config/aarch64/iterators.md
+++ b/gcc/config/aarch64/iterators.md
@@ -2804,6 +2804,8 @@
(define_code_iterator FMAXMIN [smax smin])
+(define_code_iterator UMAXMIN [umax umin])
+
;; Signed and unsigned max operations.
(define_code_iterator USMAX [smax umax])
@@ -3092,6 +3094,9 @@
(define_code_attr maxminand [(smax "bic") (smin "and")])
+(define_code_attr ovf_add_cmp [(umax "geu") (umin "ltu")])
+(define_code_attr ovf_sub_cmp [(umax "ltu") (umin "geu")])
+
;; MLA/MLS attributes.
(define_code_attr as [(ss_plus "a") (ss_minus "s")])
@@ -5007,3 +5012,7 @@
(UNSPEC_F2CVT "f2cvt")
(UNSPEC_F1CVTLT "f1cvtlt")
(UNSPEC_F2CVTLT "f2cvtlt")])
+
+;; Operand numbers for commutative operations
+(define_int_iterator ovf_commutate [1 2])
Note that this iterator already exists with the name BSL_DUP. It might
be better to rename it to something more general and re-purpose it here,
rather than duplicating it.
Yeah, I saw that as well - using BSL_DUP and bsl_mov would achieve the same
purpose, but I am not sure what commonality between my usage and BSL can be
used for naming? ONE_TWO / two_one are the obvious names, but they don't seem
very good.
Yeah, those are the names that came to mind for me, too, but it's
arguably less readable in context, so IMO it's OK to duplicate the
iterator for the sake of better readability (as the current patch does),
if you prefer.
+(define_int_attr ovf_comm_opp [(1 "2") (2 "1")])
diff --git a/gcc/testsuite/gcc.target/aarch64/pr116815-1.c
b/gcc/testsuite/gcc.target/aarch64/pr116815-1.c
new file mode 100644
index 00000000000..f3bdb794318
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/pr116815-1.c
@@ -0,0 +1,120 @@
+/* { dg-do compile } */
+/* { dg-options "-O2" } */
+/* { dg-final { check-function-bodies "**" "" "" } } */
+
+/* PR middle-end/116815 */
+
+/* Single-use tests. */
+
+static inline unsigned __attribute__ ((always_inline))
+max (unsigned a, unsigned b)
+{
+ return a > b ? a : b;
+}
+
+static inline unsigned __attribute__ ((always_inline))
+min (unsigned a, unsigned b)
+{
+ return a < b ? a : b;
+}
+
+#define OPERATION(op, type, N, exp1, exp2)
\
+ unsigned u##op##type##N (unsigned a, unsigned b) { return op (exp1, exp2); }
+
+/*
+** umaxadd1:
+** adds (w[0-9]+), w0, w1
+** csel w0, \1, w0, cc
+** ret
+*/
+OPERATION (max, add, 1, a, a + b)
+
+/*
+** umaxadd2:
+** adds (w[0-9]+), w0, w1
+** csel w0, \1, w0, cc
+** ret
+*/
+OPERATION (max, add, 2, a, b + a)
+
+/*
+** umaxadd3:
+** adds (w[0-9]+), w0, w1
+** csel w0, \1, w0, cc
+** ret
+*/
+OPERATION (max, add, 3, a + b, a)
+
+/*
+** umaxadd4:
+** adds (w[0-9]+), w0, w1
+** csel w0, \1, w0, cc
+** ret
+*/
+OPERATION (max, add, 4, b + a, a)
+
+/*
+** uminadd1:
+** adds (w[0-9]+), w0, w1
+** csel w0, \1, w0, cs
+** ret
+*/
+OPERATION (min, add, 1, a, a + b)
+
+/*
+** uminadd2:
+** adds (w[0-9]+), w0, w1
+** csel w0, \1, w0, cs
+** ret
+*/
+OPERATION (min, add, 2, a, b + a)
+
+/*
+** uminadd3:
+** adds (w[0-9]+), w0, w1
+** csel w0, \1, w0, cs
+** ret
+*/
+OPERATION (min, add, 3, a + b, a)
+
+/*
+** uminadd4:
+** adds (w[0-9]+), w0, w1
+** csel w0, \1, w0, cs
+** ret
+*/
+OPERATION (min, add, 4, b + a, a)
+
+/* sub requires the inverse of the comparison from add. */
+
+/*
+** umaxsub1:
+** subs (w[0-9]+), w0, w1
+** csel w0, \1, w0, cc
+** ret
+*/
+OPERATION (max, sub, 1, a, a - b)
+
+/*
+** umaxsub2:
+** subs (w[0-9]+), w0, w1
+** csel w0, \1, w0, cc
+** ret
+*/
+OPERATION (max, sub, 2, a - b, a)
+
+/*
+** uminsub1:
+** subs (w[0-9]+), w0, w1
+** csel w0, \1, w0, cs
+** ret
+*/
+OPERATION (min, sub, 1, a, a - b)
+
+/*
+** uminsub2:
+** subs (w[0-9]+), w0, w1
+** csel w0, \1, w0, cs
+** ret
+*/
+OPERATION (min, sub, 2, a - b, a)
diff --git a/gcc/testsuite/gcc.target/aarch64/pr116815-2.c
b/gcc/testsuite/gcc.target/aarch64/pr116815-2.c
new file mode 100644
index 00000000000..f2dd7b0bbb3
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/pr116815-2.c
@@ -0,0 +1,93 @@
+/* { dg-do compile } */
+/* { dg-options "-O2" } */
+
+/* PR middle-end/116815 */
+
+/* Negative tests. */
I don't see the benefit in including negative tests here, the tests will
get optimized out by the middle-end anyway due to C overflow rules.
Sure. I have removed this testcase in the new revision.
+
+static inline int __attribute__ ((always_inline))
+smax (int a, int b)
+{
+ return a > b ? a : b;
+}
+
+static inline int __attribute__ ((always_inline))
+smin (int a, int b)
+{
+ return a < b ? a : b;
+}
+
+static inline unsigned __attribute__ ((always_inline))
+umax (unsigned a, unsigned b)
+{
+ return a > b ? a : b;
+}
+
+static inline unsigned __attribute__ ((always_inline))
+umin (unsigned a, unsigned b)
+{
+ return a < b ? a : b;
+}
+
+#define ASSUME(cond) if (!(cond)) __builtin_unreachable ();
+
+/* This transformation does not trigger on signed types. */
+
+int
+smax_add (int a, int b)
+{
+ ASSUME (b >= 0);
+ return smax (a, a + b);
+}
+
+int
+smin_add (int a, int b)
+{
+ ASSUME (b >= 0);
+ return smin (a, a + b);
+}
+
+int
+smax_sub (int a, int b)
+{
+ ASSUME (b >= 0);
+ return smax (a, a - b);
+}
+
+int
+smin_sub (int a, int b)
+{
+ ASSUME (b >= 0);
+ return smin (a, a - b);
+}
+
+/* Invalid patterns. */
+
+/* This can potentially be matched, but the RHS gets factored to
+ (a + b) * b. */
+unsigned
+umax_factored (unsigned a, unsigned b)
+{
+ return umax (a * b, a * b + b * b);
+}
+
+unsigned
+umin_mult (unsigned a, unsigned b)
+{
+ return umin (a, a * b);
+}
+
+unsigned
+umax_sub (unsigned a, unsigned b)
+{
+ return umax (a, b - a);
+}
+
+unsigned
+umin_sub (unsigned a, unsigned b)
+{
+ return umin (a, b - a);
+}
+
+/* { dg-final { scan-assembler-not "adds\\t" } } */
+/* { dg-final { scan-assembler-not "subs\\t" } } */
diff --git a/gcc/testsuite/gcc.target/aarch64/pr116815-3.c
b/gcc/testsuite/gcc.target/aarch64/pr116815-3.c
new file mode 100644
index 00000000000..f8050377116
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/pr116815-3.c
@@ -0,0 +1,62 @@
+/* { dg-do compile } */
+/* { dg-options "-O2" } */
+/* { dg-final { check-function-bodies "**" "" "" } } */
+
+/* PR middle-end/116815 */
+
+/* Multi-use tests. */
+
+static inline unsigned __attribute__ ((always_inline))
+max (unsigned a, unsigned b)
+{
+ return a > b ? a : b;
+}
+
+static inline unsigned __attribute__ ((always_inline))
+min (unsigned a, unsigned b)
+{
+ return a < b ? a : b;
+}
+
+/* FIXME: This should only generate one adds. */
I appreciate you raising the shortcomings of the patch, but perhaps better just
to mention it in the cover letter rather than adding a test that explicitly
defends the suboptimal codegen. AIUI, check-function-bodies tests are intended
for when we're confident there is one optimal sequence of codegen that we want
to generate.
I have added the FIXME as a comment in the commit message, does that seem okay?
Yep, that's fine.
This testcase is actually a leftover from when the patch was a GIMPLE transform,
because this used to work before and is a regression after switching to a
target-specific transform. I have removed this testcase now.
I see, thanks.
<snip>
From 588dca91b719168f8ff98b7e8ee4b395bccc7cda Mon Sep 17 00:00:00 2001
From: Dhruv Chawla <[email protected]>
Date: Wed, 23 Jul 2025 01:41:51 -0700
Subject: [PATCH] [aarch64] Make better use of overflowing operations in
max/min(a, add/sub(a, b)) [PR116815]
This patch folds the following patterns:
- For add:
- umax (a, add (a, b)) -> [sum, ovf] = adds (a, b); !ovf ? sum : a
- umin (a, add (a, b)) -> [sum, ovf] = adds (a, b); !ovf ? a : sum
... along with the commutated versions:
- umax (a, add (b, a)) -> [sum, ovf] = adds (b, a); !ovf ? sum : a
- umin (a, add (b, a)) -> [sum, ovf] = adds (b, a); !ovf ? a : sum
- For sub:
- umax (a, sub (a, b)) -> [diff, udf] = subs (a, b); udf ? diff : a
- umin (a, sub (a, b)) -> [diff, udf] = subs (a, b); udf ? a : diff
Where ovf is the overflow flag and udf is the underflow flag. adds and subs are
generated by generating parallel compare+plus/minus which map to
add<mode>3_compareC and sub<mode>3_compare1.
This patch is a respin of the patch posted at
https://gcc.gnu.org/pipermail/gcc-patches/2025-May/685021.html as per
the suggestion to turn it into a target-specific transform by Richard
Biener.
FIXME: This pattern cannot currently factor multiple occurences of the
add expression into a single adds, eg: max (a, a + b) + min (a + b, b)
ends up generating two adds instructions. This is something that
was lost when going from GIMPLE to target-specific transforms.
Bootstrapped and regtested on aarch64-unknown-linux-gnu.
Signed-off-by: Dhruv Chawla <[email protected]>
PR middle-end/116815
gcc/ChangeLog:
* config/aarch64/aarch64.md
(*aarch64_plus_within_<optab><mode>3_<ovf_commutate>): New pattern.
(*aarch64_minus_within_<optab><mode>3): Likewise.
* config/aarch64/iterators.md (ovf_add_cmp): New code attribute.
(udf_sub_cmp): Likewise.
(UMAXMIN): New code iterator.
(ovf_commutate): New iterator.
(ovf_comm_opp): New int attribute.
gcc/testsuite/ChangeLog:
* gcc.target/aarch64/pr116815-1.c: New test.
* gcc.target/aarch64/pr116815-2.c: Likewise.
* gcc.target/aarch64/pr116815-3.c: Likewise.
* gcc.target/aarch64/pr116815-4.c: Likewise.
---
gcc/config/aarch64/aarch64.md | 60 +++++++++
gcc/config/aarch64/iterators.md | 9 ++
gcc/testsuite/gcc.target/aarch64/pr116815-1.c | 120 ++++++++++++++++++
gcc/testsuite/gcc.target/aarch64/pr116815-2.c | 48 +++++++
gcc/testsuite/gcc.target/aarch64/pr116815-3.c | 44 +++++++
gcc/testsuite/gcc.target/aarch64/pr116815-4.c | 60 +++++++++
6 files changed, 341 insertions(+)
create mode 100644 gcc/testsuite/gcc.target/aarch64/pr116815-1.c
create mode 100644 gcc/testsuite/gcc.target/aarch64/pr116815-2.c
create mode 100644 gcc/testsuite/gcc.target/aarch64/pr116815-3.c
create mode 100644 gcc/testsuite/gcc.target/aarch64/pr116815-4.c
diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md
index 98c65a74c8e..307df4e92ba 100644
--- a/gcc/config/aarch64/aarch64.md
+++ b/gcc/config/aarch64/aarch64.md
@@ -4488,6 +4488,66 @@
[(set_attr "type" "<su>div")]
)
+;; umax (a, add (a, b)) => [sum, ovf] = adds (a, b); !ovf ? sum : a
+;; umin (a, add (a, b)) => [sum, ovf] = adds (a, b); !ovf ? a : sum
+;; ... and the commutated versions:
+;; umax (a, add (b, a)) => [sum, ovf] = adds (b, a); !ovf ? sum : a
+;; umin (a, add (b, a)) => [sum, ovf] = adds (b, a); !ovf ? a : sum
+(define_insn_and_split "*aarch64_plus_within_<optab><mode>3_<ovf_commutate>"
+ [(set (match_operand:GPI 0 "register_operand" "=r")
+ (UMAXMIN:GPI
+ (plus:GPI (match_operand:GPI 1 "register_operand" "r")
+ (match_operand:GPI 2 "register_operand" "r"))
+ (match_dup ovf_commutate)))
+ (clobber (match_scratch:GPI 3))]
Please add an "=r" constraint here.
+ "!TARGET_CSSC"
+ "#"
+ "&& !reload_completed"
This should be "&& 1".
+ [(parallel
+ [(set (reg:CC_C CC_REGNUM)
+ (compare:CC_C (plus:GPI (match_dup ovf_commutate)
+ (match_dup <ovf_comm_opp>))
+ (match_dup ovf_commutate)))
+ (set (match_dup 3) (plus:GPI (match_dup ovf_commutate)
+ (match_dup <ovf_comm_opp>)))])
+ (set (match_dup 0)
+ (if_then_else:GPI (<ovf_add_cmp> (reg:CC_C CC_REGNUM)
+ (const_int 0))
+ (match_dup 3)
+ (match_dup ovf_commutate)))]
+ {
+ if (GET_CODE (operands[3]) == SCRATCH)
+ operands[3] = gen_reg_rtx (<MODE>mode);
+ }
+)
+
+;; umax (a, sub (a, b)) => [diff, udf] = subs (a, b); udf ? diff : a
+;; umin (a, sub (a, b)) => [diff, udf] = subs (a, b); udf ? a : diff
+(define_insn_and_split "*aarch64_minus_within_<optab><mode>3"
+ [(set (match_operand:GPI 0 "register_operand" "=r")
+ (UMAXMIN:GPI
+ (minus:GPI (match_operand:GPI 1 "register_operand" "r")
+ (match_operand:GPI 2 "register_operand" "r"))
+ (match_dup 1)))
+ (clobber (match_scratch:GPI 3))]
Please add an "=r" constraint here.
+ "!TARGET_CSSC"
+ "#"
+ "&& !reload_completed"
This should be "&& 1".
+ [(parallel
+ [(set (reg:CC CC_REGNUM)
+ (compare:CC (match_dup 1) (match_dup 2)))
+ (set (match_dup 3) (minus:GPI (match_dup 1) (match_dup 2)))])
+ (set (match_dup 0)
+ (if_then_else:GPI (<udf_sub_cmp> (reg:CC CC_REGNUM)
+ (const_int 0))
+ (match_dup 3)
+ (match_dup 1)))]
+ {
+ if (GET_CODE (operands[3]) == SCRATCH)
+ operands[3] = gen_reg_rtx (<MODE>mode);
+ }
+)
+
;; -------------------------------------------------------------------
;; Comparison insns
;; -------------------------------------------------------------------
diff --git a/gcc/config/aarch64/iterators.md b/gcc/config/aarch64/iterators.md
index 517b2808b5f..74186cfae8c 100644
--- a/gcc/config/aarch64/iterators.md
+++ b/gcc/config/aarch64/iterators.md
@@ -2827,6 +2827,8 @@
(define_code_iterator FMAXMIN [smax smin])
+(define_code_iterator UMAXMIN [umax umin])
+
;; Signed and unsigned max operations.
(define_code_iterator USMAX [smax umax])
@@ -3115,6 +3117,9 @@
(define_code_attr maxminand [(smax "bic") (smin "and")])
+(define_code_attr ovf_add_cmp [(umax "geu") (umin "ltu")])
+(define_code_attr udf_sub_cmp [(umax "ltu") (umin "geu")])
+
;; MLA/MLS attributes.
(define_code_attr as [(ss_plus "a") (ss_minus "s")])
@@ -5126,3 +5131,7 @@
(UNSPEC_F2CVT "f2cvt")
(UNSPEC_F1CVTLT "f1cvtlt")
(UNSPEC_F2CVTLT "f2cvtlt")])
+
+;; Operand numbers for commutative operations
+(define_int_iterator ovf_commutate [1 2])
+(define_int_attr ovf_comm_opp [(1 "2") (2 "1")])
diff --git a/gcc/testsuite/gcc.target/aarch64/pr116815-1.c
b/gcc/testsuite/gcc.target/aarch64/pr116815-1.c
new file mode 100644
index 00000000000..f3bdb794318
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/pr116815-1.c
@@ -0,0 +1,120 @@
+/* { dg-do compile } */
+/* { dg-options "-O2" } */
+/* { dg-final { check-function-bodies "**" "" "" } } */
+
+/* PR middle-end/116815 */
+
+/* Single-use tests. */
+
+static inline unsigned __attribute__ ((always_inline))
+max (unsigned a, unsigned b)
+{
+ return a > b ? a : b;
+}
+
+static inline unsigned __attribute__ ((always_inline))
+min (unsigned a, unsigned b)
+{
+ return a < b ? a : b;
+}
+
+#define OPERATION(op, type, N, exp1, exp2)
\
+ unsigned u##op##type##N (unsigned a, unsigned b) { return op (exp1, exp2); }
+
+/*
+** umaxadd1:
+** adds (w[0-9]+), w0, w1
+** csel w0, \1, w0, cc
+** ret
+*/
+OPERATION (max, add, 1, a, a + b)
+
+/*
+** umaxadd2:
+** adds (w[0-9]+), w0, w1
+** csel w0, \1, w0, cc
+** ret
+*/
+OPERATION (max, add, 2, a, b + a)
+
+/*
+** umaxadd3:
+** adds (w[0-9]+), w0, w1
+** csel w0, \1, w0, cc
+** ret
+*/
+OPERATION (max, add, 3, a + b, a)
+
+/*
+** umaxadd4:
+** adds (w[0-9]+), w0, w1
+** csel w0, \1, w0, cc
+** ret
+*/
+OPERATION (max, add, 4, b + a, a)
+
+/*
+** uminadd1:
+** adds (w[0-9]+), w0, w1
+** csel w0, \1, w0, cs
+** ret
+*/
+OPERATION (min, add, 1, a, a + b)
+
+/*
+** uminadd2:
+** adds (w[0-9]+), w0, w1
+** csel w0, \1, w0, cs
+** ret
+*/
+OPERATION (min, add, 2, a, b + a)
+
+/*
+** uminadd3:
+** adds (w[0-9]+), w0, w1
+** csel w0, \1, w0, cs
+** ret
+*/
+OPERATION (min, add, 3, a + b, a)
+
+/*
+** uminadd4:
+** adds (w[0-9]+), w0, w1
+** csel w0, \1, w0, cs
+** ret
+*/
+OPERATION (min, add, 4, b + a, a)
+
+/* sub requires the inverse of the comparison from add. */
+
+/*
+** umaxsub1:
+** subs (w[0-9]+), w0, w1
+** csel w0, \1, w0, cc
+** ret
+*/
+OPERATION (max, sub, 1, a, a - b)
+
+/*
+** umaxsub2:
+** subs (w[0-9]+), w0, w1
+** csel w0, \1, w0, cc
+** ret
+*/
+OPERATION (max, sub, 2, a - b, a)
+
+/*
+** uminsub1:
+** subs (w[0-9]+), w0, w1
+** csel w0, \1, w0, cs
+** ret
+*/
+OPERATION (min, sub, 1, a, a - b)
+
+/*
+** uminsub2:
+** subs (w[0-9]+), w0, w1
+** csel w0, \1, w0, cs
+** ret
+*/
+OPERATION (min, sub, 2, a - b, a)
diff --git a/gcc/testsuite/gcc.target/aarch64/pr116815-2.c
b/gcc/testsuite/gcc.target/aarch64/pr116815-2.c
new file mode 100644
index 00000000000..eb1fdb94797
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/pr116815-2.c
@@ -0,0 +1,48 @@
+/* { dg-do compile } */
+/* { dg-options "-O2" } */
+
+/* PR middle-end/116815 */
+
+/* Multi-use testcase across basic blocks. */
+
+static inline unsigned __attribute__ ((always_inline))
+max (unsigned a, unsigned b)
+{
+ return a > b ? a : b;
+}
+
+static inline unsigned __attribute__ ((always_inline))
+min (unsigned a, unsigned b)
+{
+ return a < b ? a : b;
+}
+
+#define OPERATION(op, type, N, exp1, exp2)
\
+ unsigned u##op##type##N (unsigned a, unsigned b, unsigned c, unsigned d,
\
+ unsigned e) \
+ {
\
+ unsigned result = op (exp1, exp2);
\
+ if (result == c || result == c * 2)
\
+ return d;
\
+ else
\
+ return e;
\
+ }
What is the purpose/intent of this test? IMO it seems superfluous.
The patch is OK without it and with the above changes to the patterns.
Thanks!
Alex
+
+OPERATION (max, add, 1, a, a + b)
+OPERATION (max, add, 2, a, b + a)
+OPERATION (max, add, 3, a + b, a)
+OPERATION (max, add, 4, b + a, a)
+
+OPERATION (min, add, 1, a, a + b)
+OPERATION (min, add, 2, a, b + a)
+OPERATION (min, add, 3, a + b, a)
+OPERATION (min, add, 4, b + a, a)
+
+OPERATION (max, sub, 1, a, a - b)
+OPERATION (max, sub, 2, a - b, a)
+
+OPERATION (min, sub, 1, a, a - b)
+OPERATION (min, sub, 2, a - b, a)
+
+/* { dg-final { scan-assembler-times "adds\\t" 8 } } */
+/* { dg-final { scan-assembler-times "subs\\t" 4 } } */
diff --git a/gcc/testsuite/gcc.target/aarch64/pr116815-3.c
b/gcc/testsuite/gcc.target/aarch64/pr116815-3.c
new file mode 100644
index 00000000000..015c868aec2
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/pr116815-3.c
@@ -0,0 +1,44 @@
+/* { dg-do compile } */
+/* { dg-options "-O2" } */
+
+#pragma GCC target "+cssc"
+
+/* PR middle-end/116815 */
+
+/* Make sure that umax/umin instructions are generated with CSSC. */
+
+static inline unsigned __attribute__ ((always_inline))
+max (unsigned a, unsigned b)
+{
+ return a > b ? a : b;
+}
+
+static inline unsigned __attribute__ ((always_inline))
+min (unsigned a, unsigned b)
+{
+ return a < b ? a : b;
+}
+
+#define OPERATION(op, type, N, exp1, exp2)
\
+ unsigned u##op##type##N (unsigned a, unsigned b) { return op (exp1, exp2); }
+
+OPERATION (max, add, 1, a, a + b)
+OPERATION (max, add, 2, a, b + a)
+OPERATION (max, add, 3, a + b, a)
+OPERATION (max, add, 4, b + a, a)
+
+OPERATION (min, add, 1, a, a + b)
+OPERATION (min, add, 2, a, b + a)
+OPERATION (min, add, 3, a + b, a)
+OPERATION (min, add, 4, b + a, a)
+
+OPERATION (max, sub, 1, a, a - b)
+OPERATION (max, sub, 2, a - b, a)
+
+OPERATION (min, sub, 1, a, a - b)
+OPERATION (min, sub, 2, a - b, a)
+
+/* { dg-final { scan-assembler-times "umax\\t" 6 } } */
+/* { dg-final { scan-assembler-times "umin\\t" 6 } } */
+/* { dg-final { scan-assembler-not "adds\\t" } } */
+/* { dg-final { scan-assembler-not "subs\\t" } } */
diff --git a/gcc/testsuite/gcc.target/aarch64/pr116815-4.c
b/gcc/testsuite/gcc.target/aarch64/pr116815-4.c
new file mode 100644
index 00000000000..d262d2170f3
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/pr116815-4.c
@@ -0,0 +1,60 @@
+/* { dg-do run } */
+/* { dg-options "-O2" } */
+
+/* PR middle-end/116815 */
+
+/* Verify that the transformation gives correct results */
+
+static inline unsigned __attribute__ ((always_inline))
+min (unsigned a, unsigned b)
+{
+ return (a < b) ? a : b;
+}
+
+static inline unsigned __attribute__ ((always_inline))
+max (unsigned a, unsigned b)
+{
+ return (a > b) ? a : b;
+}
+
+__attribute__ ((noipa)) unsigned
+umaxadd (unsigned a, unsigned b)
+{
+ return max (a + b, a);
+}
+
+__attribute__ ((noipa)) unsigned
+umaxsub (unsigned a, unsigned b)
+{
+ return max (a - b, a);
+}
+
+__attribute__ ((noipa)) unsigned
+uminadd (unsigned a, unsigned b)
+{
+ return min (a + b, a);
+}
+
+__attribute__ ((noipa)) unsigned
+uminsub (unsigned a, unsigned b)
+{
+ return min (a - b, a);
+}
+
+int
+main ()
+{
+ /* Overflows to 0x30000000. */
+ if (umaxadd (0x90000000, 0xa0000000) != 0x90000000)
+ __builtin_abort ();
+
+ if (uminadd (0x90000000, 0xa0000000) != 0x30000000)
+ __builtin_abort ();
+
+ /* Underflows to 0x60000000. */
+ if (umaxsub (0x00000000, 0xa0000000) != 0x60000000)
+ __builtin_abort ();
+
+ if (uminsub (0x00000000, 0xa0000000) != 0x00000000)
+ __builtin_abort ();
+}
--
2.44.0
