This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.

Index Nav:	[Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav:	[Date Prev] [Date Next]	[Thread Prev] [Thread Next]
Other format:	[Raw text]

Re: [PATCH][AArch64][v2] Improve comparison with complex immediates followed by branch/cset

From: Kyrill Tkachov <kyrylo dot tkachov at arm dot com>
To: James Greenhalgh <james dot greenhalgh at arm dot com>
Cc: GCC Patches <gcc-patches at gcc dot gnu dot org>, Marcus Shawcroft <marcus dot shawcroft at arm dot com>, Richard Earnshaw <Richard dot Earnshaw at arm dot com>
Date: Tue, 24 Nov 2015 09:42:29 +0000
Subject: Re: [PATCH][AArch64][v2] Improve comparison with complex immediates followed by branch/cset
Authentication-results: sourceware.org; auth=none
References: <5638D61C dot 5060100 at arm dot com> <20151112120543 dot GA22716 at arm dot com> <5652EB5D dot 8040002 at arm dot com> <20151123145800 dot GB14088 at arm dot com> <56532A43 dot 3000500 at arm dot com>


On 23/11/15 15:01, Kyrill Tkachov wrote:


On 23/11/15 14:58, James Greenhalgh wrote:

On Mon, Nov 23, 2015 at 10:33:01AM +0000, Kyrill Tkachov wrote:

On 12/11/15 12:05, James Greenhalgh wrote:

On Tue, Nov 03, 2015 at 03:43:24PM +0000, Kyrill Tkachov wrote:

Hi all,

Bootstrapped and tested on aarch64.

Ok for trunk?

Comments in-line.

Here's an updated patch according to your comments.
Sorry it took so long to respin it, had other things to deal with with
stage1 closing...

I've indented the sample code sequences and used valid mnemonics.
These patterns can only match during combine, so I'd expect them to always
split during combine or immediately after, but I don't think that's a documented
guarantee so I've gated them on !reload_completed.

I've used IN_RANGE in the predicate.md hunk and added scan-assembler checks
in the tests.

Is this ok?

Thanks,
Kyrill

2015-11-20  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>

     * config/aarch64/aarch64.md (*condjump): Rename to...
     (condjump): ... This.
     (*compare_condjump<mode>): New define_insn_and_split.
     (*compare_cstore<mode>_insn): Likewise.
     (*cstore<mode>_insn): Rename to...
     (cstore<mode>_insn): ... This.
     * config/aarch64/iterators.md (CMP): Handle ne code.
     * config/aarch64/predicates.md (aarch64_imm24): New predicate.

2015-11-20  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>

     * gcc.target/aarch64/cmpimm_branch_1.c: New test.
     * gcc.target/aarch64/cmpimm_cset_1.c: Likewise.

commit bb44feed4e6beaae25d9bdffa45073dc61c65838
Author: Kyrylo Tkachov <kyrylo.tkachov@arm.com>
Date:   Mon Sep 21 10:56:47 2015 +0100

     [AArch64] Improve comparison with complex immediates

diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md
index 11f6387..3e57d08 100644
--- a/gcc/config/aarch64/aarch64.md
+++ b/gcc/config/aarch64/aarch64.md
@@ -372,7 +372,7 @@ (define_expand "mod<mode>3"
    }
  )
  -(define_insn "*condjump"
+(define_insn "condjump"
    [(set (pc) (if_then_else (match_operator 0 "aarch64_comparison_operator"
                  [(match_operand 1 "cc_register" "") (const_int 0)])
                 (label_ref (match_operand 2 "" ""))
@@ -397,6 +397,41 @@ (define_insn "*condjump"
                (const_int 1)))]
  )
  +;; For a 24-bit immediate CST we can optimize the compare for equality
+;; and branch sequence from:
+;;     mov    x0, #imm1
+;;     movk    x0, #imm2, lsl 16 /* x0 contains CST.  */
+;;     cmp    x1, x0
+;;     b<ne,eq> .Label
+;; into the shorter:
+;;     sub    x0, x0, #(CST & 0xfff000)
+;;     subs    x0, x0, #(CST & 0x000fff)

   sub x0, x1, #(CST....)  ?

The transform doesn't make sense otherwise.


Doh, yes. The source should be x1 of course.


Here's what I'll be committing.

Thanks,
Kyrill
2015-11-24  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>

    * config/aarch64/aarch64.md (*condjump): Rename to...
    (condjump): ... This.
    (*compare_condjump<mode>): New define_insn_and_split.
    (*compare_cstore<mode>_insn): Likewise.
    (*cstore<mode>_insn): Rename to...
    (cstore<mode>_insn): ... This.
    * config/aarch64/iterators.md (CMP): Handle ne code.
    * config/aarch64/predicates.md (aarch64_imm24): New predicate.

2015-11-24  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>

    * gcc.target/aarch64/cmpimm_branch_1.c: New test.
    * gcc.target/aarch64/cmpimm_cset_1.c: Likewise.

Kyrill

+;;     b<ne,eq> .Label
+(define_insn_and_split "*compare_condjump<mode>"
+  [(set (pc) (if_then_else (EQL
+                  (match_operand:GPI 0 "register_operand" "r")
+                  (match_operand:GPI 1 "aarch64_imm24" "n"))
+               (label_ref:P (match_operand 2 "" ""))
+               (pc)))]
+  "!aarch64_move_imm (INTVAL (operands[1]), <MODE>mode)
+   && !aarch64_plus_operand (operands[1], <MODE>mode)
+   && !reload_completed"
+  "#"
+  "&& true"
+  [(const_int 0)]
+  {
+    HOST_WIDE_INT lo_imm = UINTVAL (operands[1]) & 0xfff;
+    HOST_WIDE_INT hi_imm = UINTVAL (operands[1]) & 0xfff000;
+    rtx tmp = gen_reg_rtx (<MODE>mode);
+    emit_insn (gen_add<mode>3 (tmp, operands[0], GEN_INT (-hi_imm)));
+    emit_insn (gen_add<mode>3_compare0 (tmp, tmp, GEN_INT (-lo_imm)));
+    rtx cc_reg = gen_rtx_REG (CC_NZmode, CC_REGNUM);
+    rtx cmp_rtx = gen_rtx_fmt_ee (<EQL:CMP>, <MODE>mode, cc_reg, const0_rtx);
+    emit_jump_insn (gen_condjump (cmp_rtx, cc_reg, operands[2]));
+    DONE;
+  }
+)
+
  (define_expand "casesi"
    [(match_operand:SI 0 "register_operand" "")    ; Index
     (match_operand:SI 1 "const_int_operand" "")    ; Lower bound
@@ -2901,7 +2936,7 @@ (define_expand "cstore<mode>4"
    "
  )
  -(define_insn "*cstore<mode>_insn"
+(define_insn "aarch64_cstore<mode>"
    [(set (match_operand:ALLI 0 "register_operand" "=r")
      (match_operator:ALLI 1 "aarch64_comparison_operator"
       [(match_operand 2 "cc_register" "") (const_int 0)]))]
@@ -2910,6 +2945,40 @@ (define_insn "*cstore<mode>_insn"
    [(set_attr "type" "csel")]
  )
  +;; For a 24-bit immediate CST we can optimize the compare for equality
+;; and branch sequence from:
+;;     mov    x0, #imm1
+;;     movk    x0, #imm2, lsl 16 /* x0 contains CST.  */
+;;     cmp    x1, x0
+;;     cset    x2, <ne,eq>
+;; into the shorter:
+;;     sub    x0, x0, #(CST & 0xfff000)
+;;     subs    x0, x0, #(CST & 0x000fff)
+;;     cset x1, <ne, eq>.

Please fix the register allocation in your shorter sequence, these
are not equivalent.

+(define_insn_and_split "*compare_cstore<mode>_insn"
+  [(set (match_operand:GPI 0 "register_operand" "=r")
+     (EQL:GPI (match_operand:GPI 1 "register_operand" "r")
+          (match_operand:GPI 2 "aarch64_imm24" "n")))]
+  "!aarch64_move_imm (INTVAL (operands[2]), <MODE>mode)
+   && !aarch64_plus_operand (operands[2], <MODE>mode)
+   && !reload_completed"
+  "#"
+  "&& true"
+  [(const_int 0)]
+  {
+    HOST_WIDE_INT lo_imm = UINTVAL (operands[2]) & 0xfff;
+    HOST_WIDE_INT hi_imm = UINTVAL (operands[2]) & 0xfff000;
+    rtx tmp = gen_reg_rtx (<MODE>mode);
+    emit_insn (gen_add<mode>3 (tmp, operands[1], GEN_INT (-hi_imm)));
+    emit_insn (gen_add<mode>3_compare0 (tmp, tmp, GEN_INT (-lo_imm)));
+    rtx cc_reg = gen_rtx_REG (CC_NZmode, CC_REGNUM);
+    rtx cmp_rtx = gen_rtx_fmt_ee (<EQL:CMP>, <MODE>mode, cc_reg, const0_rtx);
+    emit_insn (gen_aarch64_cstore<mode> (operands[0], cmp_rtx, cc_reg));
+    DONE;
+  }
+  [(set_attr "type" "csel")]
+)
+
  ;; zero_extend version of the above
  (define_insn "*cstoresi_insn_uxtw"
    [(set (match_operand:DI 0 "register_operand" "=r")
diff --git a/gcc/config/aarch64/iterators.md b/gcc/config/aarch64/iterators.md
index c2eb7de..422bc87 100644
--- a/gcc/config/aarch64/iterators.md
+++ b/gcc/config/aarch64/iterators.md
@@ -824,7 +824,8 @@ (define_code_attr cmp_2   [(lt "1") (le "1") (eq "2") (ge "2") (gt "2")
                 (ltu "1") (leu "1") (geu "2") (gtu "2")])
    (define_code_attr CMP [(lt "LT") (le "LE") (eq "EQ") (ge "GE") (gt "GT")
-               (ltu "LTU") (leu "LEU") (geu "GEU") (gtu "GTU")])
+            (ltu "LTU") (leu "LEU") (ne "NE") (geu "GEU")
+            (gtu "GTU")])
    (define_code_attr fix_trunc_optab [(fix "fix_trunc")
                     (unsigned_fix "fixuns_trunc")])
diff --git a/gcc/config/aarch64/predicates.md b/gcc/config/aarch64/predicates.md
index e7f76e0..c0c3ff5 100644
--- a/gcc/config/aarch64/predicates.md
+++ b/gcc/config/aarch64/predicates.md
@@ -145,6 +145,11 @@ (define_predicate "aarch64_imm3"
    (and (match_code "const_int")
         (match_test "(unsigned HOST_WIDE_INT) INTVAL (op) <= 4")))
  +;; An immediate that fits into 24 bits.
+(define_predicate "aarch64_imm24"
+  (and (match_code "const_int")
+       (match_test "IN_RANGE (UINTVAL (op), 0, 0xffffff)")))
+
  (define_predicate "aarch64_pwr_imm3"
    (and (match_code "const_int")
         (match_test "INTVAL (op) != 0
diff --git a/gcc/testsuite/gcc.target/aarch64/cmpimm_branch_1.c b/gcc/testsuite/gcc.target/aarch64/cmpimm_branch_1.c
new file mode 100644
index 0000000..7ad736b
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/cmpimm_branch_1.c
@@ -0,0 +1,26 @@
+/* { dg-do compile } */
+/* { dg-options "-save-temps -O2" } */
+
+/* Test that we emit a sub+subs sequence rather than mov+movk+cmp.  */
+
+void g (void);
+void
+foo (int x)
+{
+  if (x != 0x123456)
+    g ();
+}
+
+void
+fool (long long x)
+{
+  if (x != 0x123456)
+    g ();
+}
+
+/* { dg-final { scan-assembler-not "cmp\tw\[0-9\]*.*" } } */
+/* { dg-final { scan-assembler-not "cmp\tx\[0-9\]*.*" } } */
+/* { dg-final { scan-assembler-times "sub\tw\[0-9\]+.*" 1 } } */
+/* { dg-final { scan-assembler-times "sub\tx\[0-9\]+.*" 1 } } */
+/* { dg-final { scan-assembler-times "subs\tw\[0-9\]+.*" 1 } } */
+/* { dg-final { scan-assembler-times "subs\tx\[0-9\]+.*" 1 } } */
diff --git a/gcc/testsuite/gcc.target/aarch64/cmpimm_cset_1.c b/gcc/testsuite/gcc.target/aarch64/cmpimm_cset_1.c
new file mode 100644
index 0000000..6a03cc9
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/cmpimm_cset_1.c
@@ -0,0 +1,23 @@
+/* { dg-do compile } */
+/* { dg-options "-save-temps -O2" } */
+
+/* Test that we emit a sub+subs sequence rather than mov+movk+cmp.  */
+
+int
+foo (int x)
+{
+  return x == 0x123456;
+}
+
+long
+fool (long x)
+{
+  return x == 0x123456;
+}
+

This test will be broken for ILP32. This should be long long.

OK with those comments fixed.


Thanks, I'll prepare an updated version.

Kyrill

Thanks,
James

commit 30cc3774824ba7fd372111a223ade075ad7c49cc
Author: Kyrylo Tkachov <kyrylo.tkachov@arm.com>
Date:   Mon Sep 21 10:56:47 2015 +0100

    [AArch64] Improve comparison with complex immediates

diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md
index 11f6387..3283cb2 100644
--- a/gcc/config/aarch64/aarch64.md
+++ b/gcc/config/aarch64/aarch64.md
@@ -372,7 +372,7 @@ (define_expand "mod<mode>3"
   }
 )
 
-(define_insn "*condjump"
+(define_insn "condjump"
   [(set (pc) (if_then_else (match_operator 0 "aarch64_comparison_operator"
 			    [(match_operand 1 "cc_register" "") (const_int 0)])
 			   (label_ref (match_operand 2 "" ""))
@@ -397,6 +397,41 @@ (define_insn "*condjump"
 		      (const_int 1)))]
 )
 
+;; For a 24-bit immediate CST we can optimize the compare for equality
+;; and branch sequence from:
+;; 	mov	x0, #imm1
+;; 	movk	x0, #imm2, lsl 16 /* x0 contains CST.  */
+;; 	cmp	x1, x0
+;; 	b<ne,eq> .Label
+;; into the shorter:
+;; 	sub	x0, x1, #(CST & 0xfff000)
+;; 	subs	x0, x0, #(CST & 0x000fff)
+;; 	b<ne,eq> .Label
+(define_insn_and_split "*compare_condjump<mode>"
+  [(set (pc) (if_then_else (EQL
+			      (match_operand:GPI 0 "register_operand" "r")
+			      (match_operand:GPI 1 "aarch64_imm24" "n"))
+			   (label_ref:P (match_operand 2 "" ""))
+			   (pc)))]
+  "!aarch64_move_imm (INTVAL (operands[1]), <MODE>mode)
+   && !aarch64_plus_operand (operands[1], <MODE>mode)
+   && !reload_completed"
+  "#"
+  "&& true"
+  [(const_int 0)]
+  {
+    HOST_WIDE_INT lo_imm = UINTVAL (operands[1]) & 0xfff;
+    HOST_WIDE_INT hi_imm = UINTVAL (operands[1]) & 0xfff000;
+    rtx tmp = gen_reg_rtx (<MODE>mode);
+    emit_insn (gen_add<mode>3 (tmp, operands[0], GEN_INT (-hi_imm)));
+    emit_insn (gen_add<mode>3_compare0 (tmp, tmp, GEN_INT (-lo_imm)));
+    rtx cc_reg = gen_rtx_REG (CC_NZmode, CC_REGNUM);
+    rtx cmp_rtx = gen_rtx_fmt_ee (<EQL:CMP>, <MODE>mode, cc_reg, const0_rtx);
+    emit_jump_insn (gen_condjump (cmp_rtx, cc_reg, operands[2]));
+    DONE;
+  }
+)
+
 (define_expand "casesi"
   [(match_operand:SI 0 "register_operand" "")	; Index
    (match_operand:SI 1 "const_int_operand" "")	; Lower bound
@@ -2901,7 +2936,7 @@ (define_expand "cstore<mode>4"
   "
 )
 
-(define_insn "*cstore<mode>_insn"
+(define_insn "aarch64_cstore<mode>"
   [(set (match_operand:ALLI 0 "register_operand" "=r")
 	(match_operator:ALLI 1 "aarch64_comparison_operator"
 	 [(match_operand 2 "cc_register" "") (const_int 0)]))]
@@ -2910,6 +2945,40 @@ (define_insn "*cstore<mode>_insn"
   [(set_attr "type" "csel")]
 )
 
+;; For a 24-bit immediate CST we can optimize the compare for equality
+;; and branch sequence from:
+;; 	mov	x0, #imm1
+;; 	movk	x0, #imm2, lsl 16 /* x0 contains CST.  */
+;; 	cmp	x1, x0
+;; 	cset	x2, <ne,eq>
+;; into the shorter:
+;; 	sub	x0, x1, #(CST & 0xfff000)
+;; 	subs	x0, x0, #(CST & 0x000fff)
+;; 	cset x2, <ne, eq>.
+(define_insn_and_split "*compare_cstore<mode>_insn"
+  [(set (match_operand:GPI 0 "register_operand" "=r")
+	 (EQL:GPI (match_operand:GPI 1 "register_operand" "r")
+		  (match_operand:GPI 2 "aarch64_imm24" "n")))]
+  "!aarch64_move_imm (INTVAL (operands[2]), <MODE>mode)
+   && !aarch64_plus_operand (operands[2], <MODE>mode)
+   && !reload_completed"
+  "#"
+  "&& true"
+  [(const_int 0)]
+  {
+    HOST_WIDE_INT lo_imm = UINTVAL (operands[2]) & 0xfff;
+    HOST_WIDE_INT hi_imm = UINTVAL (operands[2]) & 0xfff000;
+    rtx tmp = gen_reg_rtx (<MODE>mode);
+    emit_insn (gen_add<mode>3 (tmp, operands[1], GEN_INT (-hi_imm)));
+    emit_insn (gen_add<mode>3_compare0 (tmp, tmp, GEN_INT (-lo_imm)));
+    rtx cc_reg = gen_rtx_REG (CC_NZmode, CC_REGNUM);
+    rtx cmp_rtx = gen_rtx_fmt_ee (<EQL:CMP>, <MODE>mode, cc_reg, const0_rtx);
+    emit_insn (gen_aarch64_cstore<mode> (operands[0], cmp_rtx, cc_reg));
+    DONE;
+  }
+  [(set_attr "type" "csel")]
+)
+
 ;; zero_extend version of the above
 (define_insn "*cstoresi_insn_uxtw"
   [(set (match_operand:DI 0 "register_operand" "=r")
diff --git a/gcc/config/aarch64/iterators.md b/gcc/config/aarch64/iterators.md
index c2eb7de..422bc87 100644
--- a/gcc/config/aarch64/iterators.md
+++ b/gcc/config/aarch64/iterators.md
@@ -824,7 +824,8 @@ (define_code_attr cmp_2   [(lt "1") (le "1") (eq "2") (ge "2") (gt "2")
 			   (ltu "1") (leu "1") (geu "2") (gtu "2")])
 
 (define_code_attr CMP [(lt "LT") (le "LE") (eq "EQ") (ge "GE") (gt "GT")
-			   (ltu "LTU") (leu "LEU") (geu "GEU") (gtu "GTU")])
+			(ltu "LTU") (leu "LEU") (ne "NE") (geu "GEU")
+			(gtu "GTU")])
 
 (define_code_attr fix_trunc_optab [(fix "fix_trunc")
 				   (unsigned_fix "fixuns_trunc")])
diff --git a/gcc/config/aarch64/predicates.md b/gcc/config/aarch64/predicates.md
index e7f76e0..c0c3ff5 100644
--- a/gcc/config/aarch64/predicates.md
+++ b/gcc/config/aarch64/predicates.md
@@ -145,6 +145,11 @@ (define_predicate "aarch64_imm3"
   (and (match_code "const_int")
        (match_test "(unsigned HOST_WIDE_INT) INTVAL (op) <= 4")))
 
+;; An immediate that fits into 24 bits.
+(define_predicate "aarch64_imm24"
+  (and (match_code "const_int")
+       (match_test "IN_RANGE (UINTVAL (op), 0, 0xffffff)")))
+
 (define_predicate "aarch64_pwr_imm3"
   (and (match_code "const_int")
        (match_test "INTVAL (op) != 0
diff --git a/gcc/testsuite/gcc.target/aarch64/cmpimm_branch_1.c b/gcc/testsuite/gcc.target/aarch64/cmpimm_branch_1.c
new file mode 100644
index 0000000..7ad736b
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/cmpimm_branch_1.c
@@ -0,0 +1,26 @@
+/* { dg-do compile } */
+/* { dg-options "-save-temps -O2" } */
+
+/* Test that we emit a sub+subs sequence rather than mov+movk+cmp.  */
+
+void g (void);
+void
+foo (int x)
+{
+  if (x != 0x123456)
+    g ();
+}
+
+void
+fool (long long x)
+{
+  if (x != 0x123456)
+    g ();
+}
+
+/* { dg-final { scan-assembler-not "cmp\tw\[0-9\]*.*" } } */
+/* { dg-final { scan-assembler-not "cmp\tx\[0-9\]*.*" } } */
+/* { dg-final { scan-assembler-times "sub\tw\[0-9\]+.*" 1 } } */
+/* { dg-final { scan-assembler-times "sub\tx\[0-9\]+.*" 1 } } */
+/* { dg-final { scan-assembler-times "subs\tw\[0-9\]+.*" 1 } } */
+/* { dg-final { scan-assembler-times "subs\tx\[0-9\]+.*" 1 } } */
diff --git a/gcc/testsuite/gcc.target/aarch64/cmpimm_cset_1.c b/gcc/testsuite/gcc.target/aarch64/cmpimm_cset_1.c
new file mode 100644
index 0000000..f6fd69f
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/cmpimm_cset_1.c
@@ -0,0 +1,23 @@
+/* { dg-do compile } */
+/* { dg-options "-save-temps -O2" } */
+
+/* Test that we emit a sub+subs sequence rather than mov+movk+cmp.  */
+
+int
+foo (int x)
+{
+  return x == 0x123456;
+}
+
+long long
+fool (long long x)
+{
+  return x == 0x123456;
+}
+
+/* { dg-final { scan-assembler-not "cmp\tw\[0-9\]*.*" } } */
+/* { dg-final { scan-assembler-not "cmp\tx\[0-9\]*.*" } } */
+/* { dg-final { scan-assembler-times "sub\tw\[0-9\]+.*" 1 } } */
+/* { dg-final { scan-assembler-times "sub\tx\[0-9\]+.*" 1 } } */
+/* { dg-final { scan-assembler-times "subs\tw\[0-9\]+.*" 1 } } */
+/* { dg-final { scan-assembler-times "subs\tx\[0-9\]+.*" 1 } } */

References:
- [PATCH][AArch64][v2] Improve comparison with complex immediates followed by branch/cset
  - From: Kyrill Tkachov
- Re: [PATCH][AArch64][v2] Improve comparison with complex immediates followed by branch/cset
  - From: James Greenhalgh
- Re: [PATCH][AArch64][v2] Improve comparison with complex immediates followed by branch/cset
  - From: Kyrill Tkachov
- Re: [PATCH][AArch64][v2] Improve comparison with complex immediates followed by branch/cset
  - From: James Greenhalgh
- Re: [PATCH][AArch64][v2] Improve comparison with complex immediates followed by branch/cset
  - From: Kyrill Tkachov

Index Nav:	[Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav:	[Date Prev] [Date Next]	[Thread Prev] [Thread Next]