Re: [PATCH][AArch64] Improve csinc/csneg/csinv opportunities on immediates
On 17/07/15 20:00, pins...@gmail.com wrote: On Jul 17, 2015, at 9:58 PM, Kyrill Tkachov kyrylo.tkac...@arm.com wrote: On 10/07/15 14:45, Kyrill Tkachov wrote: On 10/07/15 10:00, pins...@gmail.com wrote: On Jul 10, 2015, at 1:47 AM, Kyrill Tkachov kyrylo.tkac...@arm.com wrote: Hi Andrew, On 10/07/15 09:40, pins...@gmail.com wrote: On Jul 10, 2015, at 1:34 AM, Kyrill Tkachov kyrylo.tkac...@arm.com wrote: Hi all, Currently when evaluating expressions like (a ? 24 : 25) we will move 24 and 25 into registers and perform a csel on them. This misses the opportunity to instead move just 24 into a register and then perform a csinc, saving us an instruction and a register use. Similarly for csneg and csinv. This patch implements that idea by allowing such pairs of immediates in *cmovmode_insn and adding an early splitter that performs the necessary transformation. The testcase included in the patch demonstrates the kind of opportunities that are now picked up. With this patch I see about 9.6% more csinc instructions being generated for SPEC2006 and the generated code looks objectively better (i.e. fewer mov-immediates and slightly lower register pressure). Bootstrapped and tested on aarch64. Ok for trunk? I think this is the wrong place for this optimization. It should happen in expr.c and we should produce cond_expr on the gimple level. I had considered it, but I wasn't sure how general the conditional increment/negate/inverse operations are to warrant a midend implementation. Do you mean the expand_cond_expr_using_cmove function in expr.c? Yes and we can expand it to even have a target hook on how to expand them if needed. I played around in that part and it seems that by the time it gets to expansion the midend doesn't have a cond_expr of the two immediates, it's a PHI node with the immediates already expanded. I have not been able to get it to match a cond_expr of two immediates there, although that could be because I'm unfamiliar with that part of the codebase. So by the time we reach expansion time we don't have a COND_EXPR of two immediates, so I tried getting the code in expr.c to do the right thing, but it didn't work out. This patch catches this opportunity at the RTL level and could catch such cases if they were to be generated by any of the pre-combine RTL passes. Or do you reckon looking for these patterns in RTL ifcvt is the way to go? I think it would be somewhat messy to express the CSNEG, CSINV opportunities there as we don't have optabs for conditional negate and invert, but conditional increment would work, though in the aarch64 case we can only do a conditional by 1 rather than a general conditional add. Right as I said, I have a patch to phiopt to produce the cond_expr when it is useful. That is create cond_expr before we even get to rtl. Ok, and if that's done, then we should be able to catch them in expand_cond_expr_using_cmove and do more optimal target-specific tricks there. If you already have something for phiopt would it be possible to send it to gcc-patches so we can play around with the expr.c part of this problem? Even a rough version would be helpful. Thanks, Kyrill Thanks, Andrew Kyrill Kyrill There is already a standard pattern for condition add so the a ? Const1 : const2 can be handled in the a generic way without much troubles. We should handle it better in rtl ifcvt too (that should be an easier patch). The neg and not cases are very target specific but can be handled by a target hook and expand it directly to it. I have patches to do both but I have not got around to cleaning them up. If anyone wants them, I can send a link to my current gcc 5.1 sources with them included. Any chance you can post them on gcc-patches even as a rough idea of what needs to be done? I posted my expr patch a few years ago but I never got around to rth's comments. This was the generic increment patch. Basically aarch64 should be implementing that pattern too. The main reason why this should be handled in gimple is that ifcvt on the rtl level is not cheap and does not catch all of the cases the simple expansion of phi-opt does. I can dig that patch up and I will be doing that next week anyways. Thanks, Andrew Thanks, Kyrill Thanks, Andrew Thanks, Kyrill 2015-07-10 Kyrylo Tkachov kyrylo.tkac...@arm.com * config/aarch64/aarch64.md (*cmovmode_insn): Move stricter check for operands 3 and 4 to pattern predicate. Allow immediates that can be expressed as csinc/csneg/csinv. New define_split. (*csinv3mode_insn): Rename to... (csinv3mode_insn): ... This. * config/aarch64/aarch64.h (AARCH64_IMMS_OK_FOR_CSNEG): New macro. (AARCH64_IMMS_OK_FOR_CSINC): Likewise. (AARCH64_IMMS_OK_FOR_CSINV): Likewise. * config/aarch64/aarch64.c (aarch64_imms_ok_for_cond_op_1): New function. (aarch64_imms_ok_for_cond_op): Likewise. *
Re: [PATCH][AArch64] Improve csinc/csneg/csinv opportunities on immediates
On 10/07/15 14:45, Kyrill Tkachov wrote: On 10/07/15 10:00, pins...@gmail.com wrote: On Jul 10, 2015, at 1:47 AM, Kyrill Tkachov kyrylo.tkac...@arm.com wrote: Hi Andrew, On 10/07/15 09:40, pins...@gmail.com wrote: On Jul 10, 2015, at 1:34 AM, Kyrill Tkachov kyrylo.tkac...@arm.com wrote: Hi all, Currently when evaluating expressions like (a ? 24 : 25) we will move 24 and 25 into registers and perform a csel on them. This misses the opportunity to instead move just 24 into a register and then perform a csinc, saving us an instruction and a register use. Similarly for csneg and csinv. This patch implements that idea by allowing such pairs of immediates in *cmovmode_insn and adding an early splitter that performs the necessary transformation. The testcase included in the patch demonstrates the kind of opportunities that are now picked up. With this patch I see about 9.6% more csinc instructions being generated for SPEC2006 and the generated code looks objectively better (i.e. fewer mov-immediates and slightly lower register pressure). Bootstrapped and tested on aarch64. Ok for trunk? I think this is the wrong place for this optimization. It should happen in expr.c and we should produce cond_expr on the gimple level. I had considered it, but I wasn't sure how general the conditional increment/negate/inverse operations are to warrant a midend implementation. Do you mean the expand_cond_expr_using_cmove function in expr.c? Yes and we can expand it to even have a target hook on how to expand them if needed. I played around in that part and it seems that by the time it gets to expansion the midend doesn't have a cond_expr of the two immediates, it's a PHI node with the immediates already expanded. I have not been able to get it to match a cond_expr of two immediates there, although that could be because I'm unfamiliar with that part of the codebase. So by the time we reach expansion time we don't have a COND_EXPR of two immediates, so I tried getting the code in expr.c to do the right thing, but it didn't work out. This patch catches this opportunity at the RTL level and could catch such cases if they were to be generated by any of the pre-combine RTL passes. Or do you reckon looking for these patterns in RTL ifcvt is the way to go? I think it would be somewhat messy to express the CSNEG, CSINV opportunities there as we don't have optabs for conditional negate and invert, but conditional increment would work, though in the aarch64 case we can only do a conditional by 1 rather than a general conditional add. Kyrill Kyrill There is already a standard pattern for condition add so the a ? Const1 : const2 can be handled in the a generic way without much troubles. We should handle it better in rtl ifcvt too (that should be an easier patch). The neg and not cases are very target specific but can be handled by a target hook and expand it directly to it. I have patches to do both but I have not got around to cleaning them up. If anyone wants them, I can send a link to my current gcc 5.1 sources with them included. Any chance you can post them on gcc-patches even as a rough idea of what needs to be done? I posted my expr patch a few years ago but I never got around to rth's comments. This was the generic increment patch. Basically aarch64 should be implementing that pattern too. The main reason why this should be handled in gimple is that ifcvt on the rtl level is not cheap and does not catch all of the cases the simple expansion of phi-opt does. I can dig that patch up and I will be doing that next week anyways. Thanks, Andrew Thanks, Kyrill Thanks, Andrew Thanks, Kyrill 2015-07-10 Kyrylo Tkachov kyrylo.tkac...@arm.com * config/aarch64/aarch64.md (*cmovmode_insn): Move stricter check for operands 3 and 4 to pattern predicate. Allow immediates that can be expressed as csinc/csneg/csinv. New define_split. (*csinv3mode_insn): Rename to... (csinv3mode_insn): ... This. * config/aarch64/aarch64.h (AARCH64_IMMS_OK_FOR_CSNEG): New macro. (AARCH64_IMMS_OK_FOR_CSINC): Likewise. (AARCH64_IMMS_OK_FOR_CSINV): Likewise. * config/aarch64/aarch64.c (aarch64_imms_ok_for_cond_op_1): New function. (aarch64_imms_ok_for_cond_op): Likewise. * config/aarch64/aarch64-protos.h (aarch64_imms_ok_for_cond_op_1): Declare prototype. (aarch64_imms_ok_for_cond_op): Likewise. 2015-07-10 Kyrylo Tkachov kyrylo.tkac...@arm.com * gcc.target/aarch64/cond-op-imm_1.c: New test. aarch64-csinc-imms.patch
Re: [PATCH][AArch64] Improve csinc/csneg/csinv opportunities on immediates
On Jul 17, 2015, at 9:58 PM, Kyrill Tkachov kyrylo.tkac...@arm.com wrote: On 10/07/15 14:45, Kyrill Tkachov wrote: On 10/07/15 10:00, pins...@gmail.com wrote: On Jul 10, 2015, at 1:47 AM, Kyrill Tkachov kyrylo.tkac...@arm.com wrote: Hi Andrew, On 10/07/15 09:40, pins...@gmail.com wrote: On Jul 10, 2015, at 1:34 AM, Kyrill Tkachov kyrylo.tkac...@arm.com wrote: Hi all, Currently when evaluating expressions like (a ? 24 : 25) we will move 24 and 25 into registers and perform a csel on them. This misses the opportunity to instead move just 24 into a register and then perform a csinc, saving us an instruction and a register use. Similarly for csneg and csinv. This patch implements that idea by allowing such pairs of immediates in *cmovmode_insn and adding an early splitter that performs the necessary transformation. The testcase included in the patch demonstrates the kind of opportunities that are now picked up. With this patch I see about 9.6% more csinc instructions being generated for SPEC2006 and the generated code looks objectively better (i.e. fewer mov-immediates and slightly lower register pressure). Bootstrapped and tested on aarch64. Ok for trunk? I think this is the wrong place for this optimization. It should happen in expr.c and we should produce cond_expr on the gimple level. I had considered it, but I wasn't sure how general the conditional increment/negate/inverse operations are to warrant a midend implementation. Do you mean the expand_cond_expr_using_cmove function in expr.c? Yes and we can expand it to even have a target hook on how to expand them if needed. I played around in that part and it seems that by the time it gets to expansion the midend doesn't have a cond_expr of the two immediates, it's a PHI node with the immediates already expanded. I have not been able to get it to match a cond_expr of two immediates there, although that could be because I'm unfamiliar with that part of the codebase. So by the time we reach expansion time we don't have a COND_EXPR of two immediates, so I tried getting the code in expr.c to do the right thing, but it didn't work out. This patch catches this opportunity at the RTL level and could catch such cases if they were to be generated by any of the pre-combine RTL passes. Or do you reckon looking for these patterns in RTL ifcvt is the way to go? I think it would be somewhat messy to express the CSNEG, CSINV opportunities there as we don't have optabs for conditional negate and invert, but conditional increment would work, though in the aarch64 case we can only do a conditional by 1 rather than a general conditional add. Right as I said, I have a patch to phiopt to produce the cond_expr when it is useful. That is create cond_expr before we even get to rtl. Thanks, Andrew Kyrill Kyrill There is already a standard pattern for condition add so the a ? Const1 : const2 can be handled in the a generic way without much troubles. We should handle it better in rtl ifcvt too (that should be an easier patch). The neg and not cases are very target specific but can be handled by a target hook and expand it directly to it. I have patches to do both but I have not got around to cleaning them up. If anyone wants them, I can send a link to my current gcc 5.1 sources with them included. Any chance you can post them on gcc-patches even as a rough idea of what needs to be done? I posted my expr patch a few years ago but I never got around to rth's comments. This was the generic increment patch. Basically aarch64 should be implementing that pattern too. The main reason why this should be handled in gimple is that ifcvt on the rtl level is not cheap and does not catch all of the cases the simple expansion of phi-opt does. I can dig that patch up and I will be doing that next week anyways. Thanks, Andrew Thanks, Kyrill Thanks, Andrew Thanks, Kyrill 2015-07-10 Kyrylo Tkachov kyrylo.tkac...@arm.com * config/aarch64/aarch64.md (*cmovmode_insn): Move stricter check for operands 3 and 4 to pattern predicate. Allow immediates that can be expressed as csinc/csneg/csinv. New define_split. (*csinv3mode_insn): Rename to... (csinv3mode_insn): ... This. * config/aarch64/aarch64.h (AARCH64_IMMS_OK_FOR_CSNEG): New macro. (AARCH64_IMMS_OK_FOR_CSINC): Likewise. (AARCH64_IMMS_OK_FOR_CSINV): Likewise. * config/aarch64/aarch64.c (aarch64_imms_ok_for_cond_op_1): New function. (aarch64_imms_ok_for_cond_op): Likewise. * config/aarch64/aarch64-protos.h (aarch64_imms_ok_for_cond_op_1): Declare prototype. (aarch64_imms_ok_for_cond_op): Likewise. 2015-07-10 Kyrylo Tkachov kyrylo.tkac...@arm.com * gcc.target/aarch64/cond-op-imm_1.c: New test. aarch64-csinc-imms.patch
[PATCH][AArch64] Improve csinc/csneg/csinv opportunities on immediates
Hi all,
Currently when evaluating expressions like (a ? 24 : 25) we will move 24 and 25
into
registers and perform a csel on them. This misses the opportunity to instead
move just 24
into a register and then perform a csinc, saving us an instruction and a
register use.
Similarly for csneg and csinv.
This patch implements that idea by allowing such pairs of immediates in
*cmovmode_insn
and adding an early splitter that performs the necessary transformation.
The testcase included in the patch demonstrates the kind of opportunities that
are now picked up.
With this patch I see about 9.6% more csinc instructions being generated for
SPEC2006
and the generated code looks objectively better (i.e. fewer mov-immediates and
slightly
lower register pressure).
Bootstrapped and tested on aarch64.
Ok for trunk?
Thanks,
Kyrill
2015-07-10 Kyrylo Tkachov kyrylo.tkac...@arm.com
* config/aarch64/aarch64.md (*cmovmode_insn): Move stricter
check for operands 3 and 4 to pattern predicate. Allow immediates
that can be expressed as csinc/csneg/csinv. New define_split.
(*csinv3mode_insn): Rename to...
(csinv3mode_insn): ... This.
* config/aarch64/aarch64.h (AARCH64_IMMS_OK_FOR_CSNEG): New macro.
(AARCH64_IMMS_OK_FOR_CSINC): Likewise.
(AARCH64_IMMS_OK_FOR_CSINV): Likewise.
* config/aarch64/aarch64.c (aarch64_imms_ok_for_cond_op_1):
New function.
(aarch64_imms_ok_for_cond_op): Likewise.
* config/aarch64/aarch64-protos.h (aarch64_imms_ok_for_cond_op_1):
Declare prototype.
(aarch64_imms_ok_for_cond_op): Likewise.
2015-07-10 Kyrylo Tkachov kyrylo.tkac...@arm.com
* gcc.target/aarch64/cond-op-imm_1.c: New test.
commit eed5af149229609215327b62b7b3b4018adc6f3f
Author: Kyrylo Tkachov kyrylo.tkac...@arm.com
Date: Wed Jul 8 10:22:20 2015 +0100
[AArch64] Improve csinc/csneg/csinv opportunities on immediates
diff --git a/gcc/config/aarch64/aarch64-protos.h b/gcc/config/aarch64/aarch64-protos.h
index 4fe437f..6e3781e 100644
--- a/gcc/config/aarch64/aarch64-protos.h
+++ b/gcc/config/aarch64/aarch64-protos.h
@@ -254,6 +254,8 @@ bool aarch64_float_const_zero_rtx_p (rtx);
bool aarch64_function_arg_regno_p (unsigned);
bool aarch64_gen_movmemqi (rtx *);
bool aarch64_gimple_fold_builtin (gimple_stmt_iterator *);
+bool aarch64_imms_ok_for_cond_op_1 (rtx, rtx);
+bool aarch64_imms_ok_for_cond_op (rtx, rtx, machine_mode);
bool aarch64_is_extend_from_extract (machine_mode, rtx, rtx);
bool aarch64_is_long_call_p (rtx);
bool aarch64_label_mentioned_p (rtx);
diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index 0d81921..8babefb 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -3268,6 +3268,36 @@ aarch64_move_imm (HOST_WIDE_INT val, machine_mode mode)
return aarch64_bitmask_imm (val, mode);
}
+/* Helper for aarch64_imms_ok_for_cond_op. */
+
+bool
+aarch64_imms_ok_for_cond_op_1 (rtx a, rtx b)
+{
+ return AARCH64_IMMS_OK_FOR_CSNEG (a, b)
+ || AARCH64_IMMS_OK_FOR_CSINC (a, b)
+ || AARCH64_IMMS_OK_FOR_CSINV (a, b);
+}
+
+/* Return true if A and B are CONST_INT rtxes that can appear in
+ the two arms of an IF_THEN_ELSE used for a CSINC, CSNEG or CSINV
+ operation in mode MODE. This is used in the splitter below
+ *cmovmode_insn in aarch64.md. */
+
+bool
+aarch64_imms_ok_for_cond_op (rtx a, rtx b, machine_mode mode)
+{
+ if (!CONST_INT_P (a) || !CONST_INT_P (b))
+return false;
+
+ /* No need to do smart splitting with constant 0. We can just do
+ normal csinc, csneg, csinv on {w,x}zr. */
+ if (a == const0_rtx || b == const0_rtx)
+return false;
+
+ return aarch64_imms_ok_for_cond_op_1 (a, b)
+ || aarch64_imms_ok_for_cond_op_1 (b, a);
+}
+
static bool
aarch64_cannot_force_const_mem (machine_mode mode ATTRIBUTE_UNUSED, rtx x)
{
diff --git a/gcc/config/aarch64/aarch64.h b/gcc/config/aarch64/aarch64.h
index 7c31376..e7aecd1 100644
--- a/gcc/config/aarch64/aarch64.h
+++ b/gcc/config/aarch64/aarch64.h
@@ -678,6 +678,15 @@ do { \
/* Maximum bytes moved by a single instruction (load/store pair). */
#define MOVE_MAX (UNITS_PER_WORD * 2)
+/* Check if CONST_INTs A and B can be used as two arguments to CSNEG. */
+#define AARCH64_IMMS_OK_FOR_CSNEG(A, B) (INTVAL (A) == -INTVAL (B))
+
+/* Check if CONST_INTs A and B can be used as two arguments to CSINC. */
+#define AARCH64_IMMS_OK_FOR_CSINC(A, B) (INTVAL (A) == (INTVAL (B) + 1))
+
+/* Check if CONST_INTs A and B can be used as two arguments to CSINV. */
+#define AARCH64_IMMS_OK_FOR_CSINV(A, B) (INTVAL (A) == ~INTVAL (B))
+
/* The base cost overhead of a memcpy call, for MOVE_RATIO and friends. */
#define AARCH64_CALL_RATIO 8
diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md
index 1e343fa..358f89b 100644
--- a/gcc/config/aarch64/aarch64.md
+++ b/gcc/config/aarch64/aarch64.md
@@ -2848,10 +2848,14 @@ (define_insn *cmovmode_insn
(if_then_else:ALLI
(match_operator 1
Re: [PATCH][AArch64] Improve csinc/csneg/csinv opportunities on immediates
Hi Andrew, On 10/07/15 09:40, pins...@gmail.com wrote: On Jul 10, 2015, at 1:34 AM, Kyrill Tkachov kyrylo.tkac...@arm.com wrote: Hi all, Currently when evaluating expressions like (a ? 24 : 25) we will move 24 and 25 into registers and perform a csel on them. This misses the opportunity to instead move just 24 into a register and then perform a csinc, saving us an instruction and a register use. Similarly for csneg and csinv. This patch implements that idea by allowing such pairs of immediates in *cmovmode_insn and adding an early splitter that performs the necessary transformation. The testcase included in the patch demonstrates the kind of opportunities that are now picked up. With this patch I see about 9.6% more csinc instructions being generated for SPEC2006 and the generated code looks objectively better (i.e. fewer mov-immediates and slightly lower register pressure). Bootstrapped and tested on aarch64. Ok for trunk? I think this is the wrong place for this optimization. It should happen in expr.c and we should produce cond_expr on the gimple level. I had considered it, but I wasn't sure how general the conditional increment/negate/inverse operations are to warrant a midend implementation. Do you mean the expand_cond_expr_using_cmove function in expr.c? I have patches to do both but I have not got around to cleaning them up. If anyone wants them, I can send a link to my current gcc 5.1 sources with them included. Any chance you can post them on gcc-patches even as a rough idea of what needs to be done? Thanks, Kyrill Thanks, Andrew Thanks, Kyrill 2015-07-10 Kyrylo Tkachov kyrylo.tkac...@arm.com * config/aarch64/aarch64.md (*cmovmode_insn): Move stricter check for operands 3 and 4 to pattern predicate. Allow immediates that can be expressed as csinc/csneg/csinv. New define_split. (*csinv3mode_insn): Rename to... (csinv3mode_insn): ... This. * config/aarch64/aarch64.h (AARCH64_IMMS_OK_FOR_CSNEG): New macro. (AARCH64_IMMS_OK_FOR_CSINC): Likewise. (AARCH64_IMMS_OK_FOR_CSINV): Likewise. * config/aarch64/aarch64.c (aarch64_imms_ok_for_cond_op_1): New function. (aarch64_imms_ok_for_cond_op): Likewise. * config/aarch64/aarch64-protos.h (aarch64_imms_ok_for_cond_op_1): Declare prototype. (aarch64_imms_ok_for_cond_op): Likewise. 2015-07-10 Kyrylo Tkachov kyrylo.tkac...@arm.com * gcc.target/aarch64/cond-op-imm_1.c: New test. aarch64-csinc-imms.patch
Re: [PATCH][AArch64] Improve csinc/csneg/csinv opportunities on immediates
On Jul 10, 2015, at 1:34 AM, Kyrill Tkachov kyrylo.tkac...@arm.com wrote: Hi all, Currently when evaluating expressions like (a ? 24 : 25) we will move 24 and 25 into registers and perform a csel on them. This misses the opportunity to instead move just 24 into a register and then perform a csinc, saving us an instruction and a register use. Similarly for csneg and csinv. This patch implements that idea by allowing such pairs of immediates in *cmovmode_insn and adding an early splitter that performs the necessary transformation. The testcase included in the patch demonstrates the kind of opportunities that are now picked up. With this patch I see about 9.6% more csinc instructions being generated for SPEC2006 and the generated code looks objectively better (i.e. fewer mov-immediates and slightly lower register pressure). Bootstrapped and tested on aarch64. Ok for trunk? I think this is the wrong place for this optimization. It should happen in expr.c and we should produce cond_expr on the gimple level. I have patches to do both but I have not got around to cleaning them up. If anyone wants them, I can send a link to my current gcc 5.1 sources with them included. Thanks, Andrew Thanks, Kyrill 2015-07-10 Kyrylo Tkachov kyrylo.tkac...@arm.com * config/aarch64/aarch64.md (*cmovmode_insn): Move stricter check for operands 3 and 4 to pattern predicate. Allow immediates that can be expressed as csinc/csneg/csinv. New define_split. (*csinv3mode_insn): Rename to... (csinv3mode_insn): ... This. * config/aarch64/aarch64.h (AARCH64_IMMS_OK_FOR_CSNEG): New macro. (AARCH64_IMMS_OK_FOR_CSINC): Likewise. (AARCH64_IMMS_OK_FOR_CSINV): Likewise. * config/aarch64/aarch64.c (aarch64_imms_ok_for_cond_op_1): New function. (aarch64_imms_ok_for_cond_op): Likewise. * config/aarch64/aarch64-protos.h (aarch64_imms_ok_for_cond_op_1): Declare prototype. (aarch64_imms_ok_for_cond_op): Likewise. 2015-07-10 Kyrylo Tkachov kyrylo.tkac...@arm.com * gcc.target/aarch64/cond-op-imm_1.c: New test. aarch64-csinc-imms.patch
Re: [PATCH][AArch64] Improve csinc/csneg/csinv opportunities on immediates
On Jul 10, 2015, at 1:47 AM, Kyrill Tkachov kyrylo.tkac...@arm.com wrote: Hi Andrew, On 10/07/15 09:40, pins...@gmail.com wrote: On Jul 10, 2015, at 1:34 AM, Kyrill Tkachov kyrylo.tkac...@arm.com wrote: Hi all, Currently when evaluating expressions like (a ? 24 : 25) we will move 24 and 25 into registers and perform a csel on them. This misses the opportunity to instead move just 24 into a register and then perform a csinc, saving us an instruction and a register use. Similarly for csneg and csinv. This patch implements that idea by allowing such pairs of immediates in *cmovmode_insn and adding an early splitter that performs the necessary transformation. The testcase included in the patch demonstrates the kind of opportunities that are now picked up. With this patch I see about 9.6% more csinc instructions being generated for SPEC2006 and the generated code looks objectively better (i.e. fewer mov-immediates and slightly lower register pressure). Bootstrapped and tested on aarch64. Ok for trunk? I think this is the wrong place for this optimization. It should happen in expr.c and we should produce cond_expr on the gimple level. I had considered it, but I wasn't sure how general the conditional increment/negate/inverse operations are to warrant a midend implementation. Do you mean the expand_cond_expr_using_cmove function in expr.c? Yes and we can expand it to even have a target hook on how to expand them if needed. There is already a standard pattern for condition add so the a ? Const1 : const2 can be handled in the a generic way without much troubles. We should handle it better in rtl ifcvt too (that should be an easier patch). The neg and not cases are very target specific but can be handled by a target hook and expand it directly to it. I have patches to do both but I have not got around to cleaning them up. If anyone wants them, I can send a link to my current gcc 5.1 sources with them included. Any chance you can post them on gcc-patches even as a rough idea of what needs to be done? I posted my expr patch a few years ago but I never got around to rth's comments. This was the generic increment patch. Basically aarch64 should be implementing that pattern too. The main reason why this should be handled in gimple is that ifcvt on the rtl level is not cheap and does not catch all of the cases the simple expansion of phi-opt does. I can dig that patch up and I will be doing that next week anyways. Thanks, Andrew Thanks, Kyrill Thanks, Andrew Thanks, Kyrill 2015-07-10 Kyrylo Tkachov kyrylo.tkac...@arm.com * config/aarch64/aarch64.md (*cmovmode_insn): Move stricter check for operands 3 and 4 to pattern predicate. Allow immediates that can be expressed as csinc/csneg/csinv. New define_split. (*csinv3mode_insn): Rename to... (csinv3mode_insn): ... This. * config/aarch64/aarch64.h (AARCH64_IMMS_OK_FOR_CSNEG): New macro. (AARCH64_IMMS_OK_FOR_CSINC): Likewise. (AARCH64_IMMS_OK_FOR_CSINV): Likewise. * config/aarch64/aarch64.c (aarch64_imms_ok_for_cond_op_1): New function. (aarch64_imms_ok_for_cond_op): Likewise. * config/aarch64/aarch64-protos.h (aarch64_imms_ok_for_cond_op_1): Declare prototype. (aarch64_imms_ok_for_cond_op): Likewise. 2015-07-10 Kyrylo Tkachov kyrylo.tkac...@arm.com * gcc.target/aarch64/cond-op-imm_1.c: New test. aarch64-csinc-imms.patch
Re: [PATCH][AArch64] Improve csinc/csneg/csinv opportunities on immediates
On 10/07/15 10:00, pins...@gmail.com wrote: On Jul 10, 2015, at 1:47 AM, Kyrill Tkachov kyrylo.tkac...@arm.com wrote: Hi Andrew, On 10/07/15 09:40, pins...@gmail.com wrote: On Jul 10, 2015, at 1:34 AM, Kyrill Tkachov kyrylo.tkac...@arm.com wrote: Hi all, Currently when evaluating expressions like (a ? 24 : 25) we will move 24 and 25 into registers and perform a csel on them. This misses the opportunity to instead move just 24 into a register and then perform a csinc, saving us an instruction and a register use. Similarly for csneg and csinv. This patch implements that idea by allowing such pairs of immediates in *cmovmode_insn and adding an early splitter that performs the necessary transformation. The testcase included in the patch demonstrates the kind of opportunities that are now picked up. With this patch I see about 9.6% more csinc instructions being generated for SPEC2006 and the generated code looks objectively better (i.e. fewer mov-immediates and slightly lower register pressure). Bootstrapped and tested on aarch64. Ok for trunk? I think this is the wrong place for this optimization. It should happen in expr.c and we should produce cond_expr on the gimple level. I had considered it, but I wasn't sure how general the conditional increment/negate/inverse operations are to warrant a midend implementation. Do you mean the expand_cond_expr_using_cmove function in expr.c? Yes and we can expand it to even have a target hook on how to expand them if needed. I played around in that part and it seems that by the time it gets to expansion the midend doesn't have a cond_expr of the two immediates, it's a PHI node with the immediates already expanded. I have not been able to get it to match a cond_expr of two immediates there, although that could be because I'm unfamiliar with that part of the codebase. Kyrill There is already a standard pattern for condition add so the a ? Const1 : const2 can be handled in the a generic way without much troubles. We should handle it better in rtl ifcvt too (that should be an easier patch). The neg and not cases are very target specific but can be handled by a target hook and expand it directly to it. I have patches to do both but I have not got around to cleaning them up. If anyone wants them, I can send a link to my current gcc 5.1 sources with them included. Any chance you can post them on gcc-patches even as a rough idea of what needs to be done? I posted my expr patch a few years ago but I never got around to rth's comments. This was the generic increment patch. Basically aarch64 should be implementing that pattern too. The main reason why this should be handled in gimple is that ifcvt on the rtl level is not cheap and does not catch all of the cases the simple expansion of phi-opt does. I can dig that patch up and I will be doing that next week anyways. Thanks, Andrew Thanks, Kyrill Thanks, Andrew Thanks, Kyrill 2015-07-10 Kyrylo Tkachov kyrylo.tkac...@arm.com * config/aarch64/aarch64.md (*cmovmode_insn): Move stricter check for operands 3 and 4 to pattern predicate. Allow immediates that can be expressed as csinc/csneg/csinv. New define_split. (*csinv3mode_insn): Rename to... (csinv3mode_insn): ... This. * config/aarch64/aarch64.h (AARCH64_IMMS_OK_FOR_CSNEG): New macro. (AARCH64_IMMS_OK_FOR_CSINC): Likewise. (AARCH64_IMMS_OK_FOR_CSINV): Likewise. * config/aarch64/aarch64.c (aarch64_imms_ok_for_cond_op_1): New function. (aarch64_imms_ok_for_cond_op): Likewise. * config/aarch64/aarch64-protos.h (aarch64_imms_ok_for_cond_op_1): Declare prototype. (aarch64_imms_ok_for_cond_op): Likewise. 2015-07-10 Kyrylo Tkachov kyrylo.tkac...@arm.com * gcc.target/aarch64/cond-op-imm_1.c: New test. aarch64-csinc-imms.patch
