| Message ID | 55A38963.6080408@arm.com |
|---|---|
| State | New |
| Headers | show |
On Mon, Jul 13, 2015 at 10:48:19AM +0100, Kyrill Tkachov wrote: > For the testcase in the patch we were generating an extra neg instruction: > cmp w0, wzr > csneg w0, w0, w0, ge > neg w0, w0 > ret > > instead of the optimal: > cmp w0, wzr > csneg w0, w0, w0, lt > ret > > The reason is that combine tries to merge the operation into a negation of > an abs. Before combine, you have two insns, a negation and an abs, so that is not so very strange :-) Some archs have actual nabs insns btw (for floating point, anyway). Archs without abs or conditional assignment, and with cheap branches, get a branch around a neg followed by another neg, at expand time. This then isn't optimised away either. So I'd say expand should be made a bit smarter for this. Failing that, your approach looks fine to me -- assuming you want to have a fake "abs" insn at all. On to the patch... > +;; Combine will try merging (c > 0 ? -x : x) into (-|x|). This isn't a good "x > 0" here. Segher
Hi Segher, On 14/07/15 01:38, Segher Boessenkool wrote: > On Mon, Jul 13, 2015 at 10:48:19AM +0100, Kyrill Tkachov wrote: >> For the testcase in the patch we were generating an extra neg instruction: >> cmp w0, wzr >> csneg w0, w0, w0, ge >> neg w0, w0 >> ret >> >> instead of the optimal: >> cmp w0, wzr >> csneg w0, w0, w0, lt >> ret >> >> The reason is that combine tries to merge the operation into a negation of >> an abs. > Before combine, you have two insns, a negation and an abs, so that is > not so very strange :-) Well, because the aarch64 integer abs expander expands to a compare and a conditional negate, we have a compare followed by an if_then_else with a neg in it. That's followed by a neg of that: (insn 6 3 7 2 (set (reg:CC 66 cc) (compare:CC (reg/v:SI 76 [ x ]) (const_int 0 [0]))) abs.c:1 374 {*cmpsi} (nil)) (insn 7 6 8 2 (set (reg:SI 78 [ D.2683 ]) (if_then_else:SI (lt (reg:CC 66 cc) (const_int 0 [0])) (neg:SI (reg/v:SI 76 [ x ])) (reg/v:SI 76 [ x ]))) abs.c:1 441 {csneg3si_insn} (expr_list:REG_DEAD (reg/v:SI 76 [ x ]) (expr_list:REG_DEAD (reg:CC 66 cc) (expr_list:REG_EQUAL (abs:SI (reg/v:SI 76 [ x ])) (nil))))) (insn 8 7 13 2 (set (reg:SI 77 [ D.2683 ]) (neg:SI (reg:SI 78 [ D.2683 ]))) abs.c:1 319 {negsi2} (expr_list:REG_DEAD (reg:SI 78 [ D.2683 ]) (nil))) combine tries to convert it into a neg-of-an-abs in simplify_if_then_else > > Some archs have actual nabs insns btw (for floating point, anyway). > > Archs without abs or conditional assignment, and with cheap branches, > get a branch around a neg followed by another neg, at expand time. > This then isn't optimised away either. > > So I'd say expand should be made a bit smarter for this. Failing > that, your approach looks fine to me -- assuming you want to have a > fake "abs" insn at all. > > On to the patch... > > >> +;; Combine will try merging (c > 0 ? -x : x) into (-|x|). This isn't a good > "x > 0" here. Thanks, I'll fix that when committing if approved. Kyrill > > Segher >
On Tue, Jul 14, 2015 at 1:18 AM, Kyrill Tkachov <kyrylo.tkachov@arm.com> wrote: > Hi Segher, > > On 14/07/15 01:38, Segher Boessenkool wrote: >> >> On Mon, Jul 13, 2015 at 10:48:19AM +0100, Kyrill Tkachov wrote: >>> >>> For the testcase in the patch we were generating an extra neg >>> instruction: >>> cmp w0, wzr >>> csneg w0, w0, w0, ge >>> neg w0, w0 >>> ret >>> >>> instead of the optimal: >>> cmp w0, wzr >>> csneg w0, w0, w0, lt >>> ret >>> >>> The reason is that combine tries to merge the operation into a negation >>> of >>> an abs. >> >> Before combine, you have two insns, a negation and an abs, so that is >> not so very strange :-) > > > Well, because the aarch64 integer abs expander expands to a compare > and a conditional negate, we have a compare followed by an if_then_else > with a neg in it. That's followed by a neg of that: > (insn 6 3 7 2 (set (reg:CC 66 cc) > (compare:CC (reg/v:SI 76 [ x ]) > (const_int 0 [0]))) abs.c:1 374 {*cmpsi} > (nil)) > (insn 7 6 8 2 (set (reg:SI 78 [ D.2683 ]) > (if_then_else:SI (lt (reg:CC 66 cc) > (const_int 0 [0])) > (neg:SI (reg/v:SI 76 [ x ])) > (reg/v:SI 76 [ x ]))) abs.c:1 441 {csneg3si_insn} > (expr_list:REG_DEAD (reg/v:SI 76 [ x ]) > (expr_list:REG_DEAD (reg:CC 66 cc) > (expr_list:REG_EQUAL (abs:SI (reg/v:SI 76 [ x ])) > (nil))))) > (insn 8 7 13 2 (set (reg:SI 77 [ D.2683 ]) > (neg:SI (reg:SI 78 [ D.2683 ]))) abs.c:1 319 {negsi2} > (expr_list:REG_DEAD (reg:SI 78 [ D.2683 ]) > (nil))) What does combine do when it props 7 into 8? I suspect you want to optimize that instead of doing it any other way. That is if prop the neg into the two sides of the conditional and if one simplifies, produce a new rtl. Thanks, Andrew Pinski > > > combine tries to convert it into a neg-of-an-abs in simplify_if_then_else > >> >> Some archs have actual nabs insns btw (for floating point, anyway). >> >> Archs without abs or conditional assignment, and with cheap branches, >> get a branch around a neg followed by another neg, at expand time. >> This then isn't optimised away either. >> >> So I'd say expand should be made a bit smarter for this. Failing >> that, your approach looks fine to me -- assuming you want to have a >> fake "abs" insn at all. >> >> On to the patch... >> >> >>> +;; Combine will try merging (c > 0 ? -x : x) into (-|x|). This isn't a >>> good >> >> "x > 0" here. > > > Thanks, I'll fix that when committing if approved. > > Kyrill > >> >> Segher >> >
On Tue, Jul 14, 2015 at 3:06 AM, Andrew Pinski <pinskia@gmail.com> wrote: > On Tue, Jul 14, 2015 at 1:18 AM, Kyrill Tkachov <kyrylo.tkachov@arm.com> wrote: >> Hi Segher, >> >> On 14/07/15 01:38, Segher Boessenkool wrote: >>> >>> On Mon, Jul 13, 2015 at 10:48:19AM +0100, Kyrill Tkachov wrote: >>>> >>>> For the testcase in the patch we were generating an extra neg >>>> instruction: >>>> cmp w0, wzr >>>> csneg w0, w0, w0, ge >>>> neg w0, w0 >>>> ret >>>> >>>> instead of the optimal: >>>> cmp w0, wzr >>>> csneg w0, w0, w0, lt >>>> ret >>>> >>>> The reason is that combine tries to merge the operation into a negation >>>> of >>>> an abs. >>> >>> Before combine, you have two insns, a negation and an abs, so that is >>> not so very strange :-) >> >> >> Well, because the aarch64 integer abs expander expands to a compare >> and a conditional negate, we have a compare followed by an if_then_else >> with a neg in it. That's followed by a neg of that: >> (insn 6 3 7 2 (set (reg:CC 66 cc) >> (compare:CC (reg/v:SI 76 [ x ]) >> (const_int 0 [0]))) abs.c:1 374 {*cmpsi} >> (nil)) >> (insn 7 6 8 2 (set (reg:SI 78 [ D.2683 ]) >> (if_then_else:SI (lt (reg:CC 66 cc) >> (const_int 0 [0])) >> (neg:SI (reg/v:SI 76 [ x ])) >> (reg/v:SI 76 [ x ]))) abs.c:1 441 {csneg3si_insn} >> (expr_list:REG_DEAD (reg/v:SI 76 [ x ]) >> (expr_list:REG_DEAD (reg:CC 66 cc) >> (expr_list:REG_EQUAL (abs:SI (reg/v:SI 76 [ x ])) >> (nil))))) >> (insn 8 7 13 2 (set (reg:SI 77 [ D.2683 ]) >> (neg:SI (reg:SI 78 [ D.2683 ]))) abs.c:1 319 {negsi2} >> (expr_list:REG_DEAD (reg:SI 78 [ D.2683 ]) >> (nil))) > > > What does combine do when it props 7 into 8? I suspect you want to > optimize that instead of doing it any other way. > That is if prop the neg into the two sides of the conditional and if > one simplifies, produce a new rtl. This should help code even like: static inline int f(int a, int b, int c) { if (a) return b; else return -c; } int g(int a, int b, int c) { return -f(a, b, c); } Note the above code might be optimized at the tree level already. Thanks, Andrew > > Thanks, > Andrew Pinski > >> >> >> combine tries to convert it into a neg-of-an-abs in simplify_if_then_else >> >>> >>> Some archs have actual nabs insns btw (for floating point, anyway). >>> >>> Archs without abs or conditional assignment, and with cheap branches, >>> get a branch around a neg followed by another neg, at expand time. >>> This then isn't optimised away either. >>> >>> So I'd say expand should be made a bit smarter for this. Failing >>> that, your approach looks fine to me -- assuming you want to have a >>> fake "abs" insn at all. >>> >>> On to the patch... >>> >>> >>>> +;; Combine will try merging (c > 0 ? -x : x) into (-|x|). This isn't a >>>> good >>> >>> "x > 0" here. >> >> >> Thanks, I'll fix that when committing if approved. >> >> Kyrill >> >>> >>> Segher >>> >>
On 14/07/15 11:06, Andrew Pinski wrote: > On Tue, Jul 14, 2015 at 1:18 AM, Kyrill Tkachov <kyrylo.tkachov@arm.com> wrote: >> Hi Segher, >> >> On 14/07/15 01:38, Segher Boessenkool wrote: >>> On Mon, Jul 13, 2015 at 10:48:19AM +0100, Kyrill Tkachov wrote: >>>> For the testcase in the patch we were generating an extra neg >>>> instruction: >>>> cmp w0, wzr >>>> csneg w0, w0, w0, ge >>>> neg w0, w0 >>>> ret >>>> >>>> instead of the optimal: >>>> cmp w0, wzr >>>> csneg w0, w0, w0, lt >>>> ret >>>> >>>> The reason is that combine tries to merge the operation into a negation >>>> of >>>> an abs. >>> Before combine, you have two insns, a negation and an abs, so that is >>> not so very strange :-) >> >> Well, because the aarch64 integer abs expander expands to a compare >> and a conditional negate, we have a compare followed by an if_then_else >> with a neg in it. That's followed by a neg of that: >> (insn 6 3 7 2 (set (reg:CC 66 cc) >> (compare:CC (reg/v:SI 76 [ x ]) >> (const_int 0 [0]))) abs.c:1 374 {*cmpsi} >> (nil)) >> (insn 7 6 8 2 (set (reg:SI 78 [ D.2683 ]) >> (if_then_else:SI (lt (reg:CC 66 cc) >> (const_int 0 [0])) >> (neg:SI (reg/v:SI 76 [ x ])) >> (reg/v:SI 76 [ x ]))) abs.c:1 441 {csneg3si_insn} >> (expr_list:REG_DEAD (reg/v:SI 76 [ x ]) >> (expr_list:REG_DEAD (reg:CC 66 cc) >> (expr_list:REG_EQUAL (abs:SI (reg/v:SI 76 [ x ])) >> (nil))))) >> (insn 8 7 13 2 (set (reg:SI 77 [ D.2683 ]) >> (neg:SI (reg:SI 78 [ D.2683 ]))) abs.c:1 319 {negsi2} >> (expr_list:REG_DEAD (reg:SI 78 [ D.2683 ]) >> (nil))) > > What does combine do when it props 7 into 8? I suspect you want to > optimize that instead of doing it any other way. > That is if prop the neg into the two sides of the conditional and if > one simplifies, produce a new rtl. Yeah, that's what combine tries in simplify_if_then_else, instead of propagating the neg it tries a (neg (abs x)). I did consider telling it not to do that, but how would it be gated? Should we look if the one arm of the if_then_else is a neg of the other and invert the comparison code instead of trying (neg (abs x)) when HAVE_conditional_move? Kyrill > > Thanks, > Andrew Pinski > >> >> combine tries to convert it into a neg-of-an-abs in simplify_if_then_else >> >>> Some archs have actual nabs insns btw (for floating point, anyway). >>> >>> Archs without abs or conditional assignment, and with cheap branches, >>> get a branch around a neg followed by another neg, at expand time. >>> This then isn't optimised away either. >>> >>> So I'd say expand should be made a bit smarter for this. Failing >>> that, your approach looks fine to me -- assuming you want to have a >>> fake "abs" insn at all. >>> >>> On to the patch... >>> >>> >>>> +;; Combine will try merging (c > 0 ? -x : x) into (-|x|). This isn't a >>>> good >>> "x > 0" here. >> >> Thanks, I'll fix that when committing if approved. >> >> Kyrill >> >>> Segher >>>
On Tue, Jul 14, 2015 at 3:13 AM, Kyrill Tkachov <kyrylo.tkachov@arm.com> wrote: > > On 14/07/15 11:06, Andrew Pinski wrote: >> >> On Tue, Jul 14, 2015 at 1:18 AM, Kyrill Tkachov <kyrylo.tkachov@arm.com> >> wrote: >>> >>> Hi Segher, >>> >>> On 14/07/15 01:38, Segher Boessenkool wrote: >>>> >>>> On Mon, Jul 13, 2015 at 10:48:19AM +0100, Kyrill Tkachov wrote: >>>>> >>>>> For the testcase in the patch we were generating an extra neg >>>>> instruction: >>>>> cmp w0, wzr >>>>> csneg w0, w0, w0, ge >>>>> neg w0, w0 >>>>> ret >>>>> >>>>> instead of the optimal: >>>>> cmp w0, wzr >>>>> csneg w0, w0, w0, lt >>>>> ret >>>>> >>>>> The reason is that combine tries to merge the operation into a negation >>>>> of >>>>> an abs. >>>> >>>> Before combine, you have two insns, a negation and an abs, so that is >>>> not so very strange :-) >>> >>> >>> Well, because the aarch64 integer abs expander expands to a compare >>> and a conditional negate, we have a compare followed by an if_then_else >>> with a neg in it. That's followed by a neg of that: >>> (insn 6 3 7 2 (set (reg:CC 66 cc) >>> (compare:CC (reg/v:SI 76 [ x ]) >>> (const_int 0 [0]))) abs.c:1 374 {*cmpsi} >>> (nil)) >>> (insn 7 6 8 2 (set (reg:SI 78 [ D.2683 ]) >>> (if_then_else:SI (lt (reg:CC 66 cc) >>> (const_int 0 [0])) >>> (neg:SI (reg/v:SI 76 [ x ])) >>> (reg/v:SI 76 [ x ]))) abs.c:1 441 {csneg3si_insn} >>> (expr_list:REG_DEAD (reg/v:SI 76 [ x ]) >>> (expr_list:REG_DEAD (reg:CC 66 cc) >>> (expr_list:REG_EQUAL (abs:SI (reg/v:SI 76 [ x ])) >>> (nil))))) >>> (insn 8 7 13 2 (set (reg:SI 77 [ D.2683 ]) >>> (neg:SI (reg:SI 78 [ D.2683 ]))) abs.c:1 319 {negsi2} >>> (expr_list:REG_DEAD (reg:SI 78 [ D.2683 ]) >>> (nil))) >> >> >> What does combine do when it props 7 into 8? I suspect you want to >> optimize that instead of doing it any other way. >> That is if prop the neg into the two sides of the conditional and if >> one simplifies, produce a new rtl. > > > Yeah, that's what combine tries in simplify_if_then_else, instead of > propagating the neg it tries a (neg (abs x)). I did consider telling it not > to do that, but how would it be gated? > Should we look if the one arm of the if_then_else is a neg of the other and > invert the comparison code instead of trying (neg (abs x)) when > HAVE_conditional_move? Does it do that even with insn 6 is not involved? I doubt it. Please look into that again. I bet it is not creating the instruction you want it to create and having the not in the last operand of the if_then_else instead of the first one. Thanks, Andrew > > Kyrill > >> >> Thanks, >> Andrew Pinski >> >>> >>> combine tries to convert it into a neg-of-an-abs in simplify_if_then_else >>> >>>> Some archs have actual nabs insns btw (for floating point, anyway). >>>> >>>> Archs without abs or conditional assignment, and with cheap branches, >>>> get a branch around a neg followed by another neg, at expand time. >>>> This then isn't optimised away either. >>>> >>>> So I'd say expand should be made a bit smarter for this. Failing >>>> that, your approach looks fine to me -- assuming you want to have a >>>> fake "abs" insn at all. >>>> >>>> On to the patch... >>>> >>>> >>>>> +;; Combine will try merging (c > 0 ? -x : x) into (-|x|). This isn't >>>>> a >>>>> good >>>> >>>> "x > 0" here. >>> >>> >>> Thanks, I'll fix that when committing if approved. >>> >>> Kyrill >>> >>>> Segher >>>> >
> On Jul 13, 2015, at 5:48 PM, Kyrill Tkachov <kyrylo.tkachov@arm.com> wrote: > > Hi all, > > For the testcase in the patch we were generating an extra neg instruction: > cmp w0, wzr > csneg w0, w0, w0, ge > neg w0, w0 > ret > > instead of the optimal: > cmp w0, wzr > csneg w0, w0, w0, lt > ret > > The reason is that combine tries to merge the operation into a negation of an abs. > I considered teaching combine not to do that but it would require telling it that it shouldn't > do it if there is a conditional negate instruction. There's no optab for that though :( > Also, we already advertise that we have an abs optab, even though we expand to a compare and > a csneg anyway. This patch was the cleanest way I could do this. We just match the neg of an abs > and generate the same csneg sequence as for normal abs, just with the comparison condition inverted. > > Bootstrapped and tested on aarch64. > > Ok for trunk? > Thanks, > Kyrill > > 2015-07-13 Kyrylo Tkachov <kyrylo.tkachov@arm.com> > > * config/aarch64/aarch64.md (*absneg2<mode>_insn): New > define_and_split. This pattern is incorrect as you need to say you are clobbering the flags register. Otherwise an optimization between combine and the splitter can move an instruction between it. Also it might be better to just have a define_split rather than a define_insn_and_split. Combine knows how to use define_split without being an insn. Thanks, Andrew > > 2015-07-13 Kyrylo Tkachov <kyrylo.tkachov@arm.com> > > * gcc.target/aarch64/neg-abs_1.c: New test. > <abs-neg.patch>
On 14/07/15 11:40, Andrew Pinski wrote: > On Tue, Jul 14, 2015 at 3:13 AM, Kyrill Tkachov <kyrylo.tkachov@arm.com> wrote: >> On 14/07/15 11:06, Andrew Pinski wrote: >>> On Tue, Jul 14, 2015 at 1:18 AM, Kyrill Tkachov <kyrylo.tkachov@arm.com> >>> wrote: >>>> Hi Segher, >>>> >>>> On 14/07/15 01:38, Segher Boessenkool wrote: >>>>> On Mon, Jul 13, 2015 at 10:48:19AM +0100, Kyrill Tkachov wrote: >>>>>> For the testcase in the patch we were generating an extra neg >>>>>> instruction: >>>>>> cmp w0, wzr >>>>>> csneg w0, w0, w0, ge >>>>>> neg w0, w0 >>>>>> ret >>>>>> >>>>>> instead of the optimal: >>>>>> cmp w0, wzr >>>>>> csneg w0, w0, w0, lt >>>>>> ret >>>>>> >>>>>> The reason is that combine tries to merge the operation into a negation >>>>>> of >>>>>> an abs. >>>>> Before combine, you have two insns, a negation and an abs, so that is >>>>> not so very strange :-) >>>> >>>> Well, because the aarch64 integer abs expander expands to a compare >>>> and a conditional negate, we have a compare followed by an if_then_else >>>> with a neg in it. That's followed by a neg of that: >>>> (insn 6 3 7 2 (set (reg:CC 66 cc) >>>> (compare:CC (reg/v:SI 76 [ x ]) >>>> (const_int 0 [0]))) abs.c:1 374 {*cmpsi} >>>> (nil)) >>>> (insn 7 6 8 2 (set (reg:SI 78 [ D.2683 ]) >>>> (if_then_else:SI (lt (reg:CC 66 cc) >>>> (const_int 0 [0])) >>>> (neg:SI (reg/v:SI 76 [ x ])) >>>> (reg/v:SI 76 [ x ]))) abs.c:1 441 {csneg3si_insn} >>>> (expr_list:REG_DEAD (reg/v:SI 76 [ x ]) >>>> (expr_list:REG_DEAD (reg:CC 66 cc) >>>> (expr_list:REG_EQUAL (abs:SI (reg/v:SI 76 [ x ])) >>>> (nil))))) >>>> (insn 8 7 13 2 (set (reg:SI 77 [ D.2683 ]) >>>> (neg:SI (reg:SI 78 [ D.2683 ]))) abs.c:1 319 {negsi2} >>>> (expr_list:REG_DEAD (reg:SI 78 [ D.2683 ]) >>>> (nil))) >>> >>> What does combine do when it props 7 into 8? I suspect you want to >>> optimize that instead of doing it any other way. >>> That is if prop the neg into the two sides of the conditional and if >>> one simplifies, produce a new rtl. >> >> Yeah, that's what combine tries in simplify_if_then_else, instead of >> propagating the neg it tries a (neg (abs x)). I did consider telling it not >> to do that, but how would it be gated? >> Should we look if the one arm of the if_then_else is a neg of the other and >> invert the comparison code instead of trying (neg (abs x)) when >> HAVE_conditional_move? > Does it do that even with insn 6 is not involved? > I doubt it. > > Please look into that again. I bet it is not creating the instruction > you want it to create and having the not in the last operand of the > if_then_else instead of the first one. Ok, I dug a bit further. There's a couple of issues here: * combine_simplify_rtx tries to simplify the comparison in the if_then_else then has the lines: if (cond_code == NE && COMPARISON_P (cond)) return x; which quits early and doesn't try optimising the (neg (if_then_else)) further. * If I bypass that condition it proceeds further into simplify_unary_operation from simplify-rtx.c which doesn't have the simplifcation rule we want. If I add a [- (x ? -y : y) -> !x ? -y : y] transformation there then it works. I'll try to whip a patch into shape. Thanks, Kyrill > > Thanks, > Andrew > > >> Kyrill >> >>> Thanks, >>> Andrew Pinski >>> >>>> combine tries to convert it into a neg-of-an-abs in simplify_if_then_else >>>> >>>>> Some archs have actual nabs insns btw (for floating point, anyway). >>>>> >>>>> Archs without abs or conditional assignment, and with cheap branches, >>>>> get a branch around a neg followed by another neg, at expand time. >>>>> This then isn't optimised away either. >>>>> >>>>> So I'd say expand should be made a bit smarter for this. Failing >>>>> that, your approach looks fine to me -- assuming you want to have a >>>>> fake "abs" insn at all. >>>>> >>>>> On to the patch... >>>>> >>>>> >>>>>> +;; Combine will try merging (c > 0 ? -x : x) into (-|x|). This isn't >>>>>> a >>>>>> good >>>>> "x > 0" here. >>>> >>>> Thanks, I'll fix that when committing if approved. >>>> >>>> Kyrill >>>> >>>>> Segher >>>>>
On Tue, Jul 14, 2015 at 07:04:13PM +0800, pinskia@gmail.com wrote:
> Combine knows how to use define_split without being an insn.
Combine uses define_split in very different circumstances than it uses
define_insn. In this case, define_split will only do anything if the
"nabs" is combined from three insns, while a define_insn handles it
being combined from two as well.
A define_split will be split during combine (and combine can then work
on its factors); a define_insn is only split _after_ combine.
Segher
On Tue, Jul 14, 2015 at 09:18:06AM +0100, Kyrill Tkachov wrote: > >Before combine, you have two insns, a negation and an abs, so that is > >not so very strange :-) Oh, hrm, my aarch64 cross was three months old, and this now changed. Or I messed up. Sorry for the noise. It does look like the if_then_else stuff in combine (and simplify-rtx) could use some improvement, probably more than just fixing this one case. Or look at the first huge case in combine_simplify_rtx, that looks eerily similar. Segher
On 14/07/15 13:55, Segher Boessenkool wrote: > On Tue, Jul 14, 2015 at 09:18:06AM +0100, Kyrill Tkachov wrote: >>> Before combine, you have two insns, a negation and an abs, so that is >>> not so very strange :-) > Oh, hrm, my aarch64 cross was three months old, and this now changed. > Or I messed up. Sorry for the noise. > > It does look like the if_then_else stuff in combine (and simplify-rtx) > could use some improvement, probably more than just fixing this one > case. Or look at the first huge case in combine_simplify_rtx, that > looks eerily similar. Yeah, removing an early return in combine_simplify_rtx and adding the simplification to simplify_unary_operation did the trick. I'll see if it survives testing. Thanks, Kyrill > > > Segher >
commit 7527a76d25067ce4a5426e563e162487604ac6c1
Author: Kyrylo Tkachov <kyrylo.tkachov@arm.com>
Date: Thu Jul 9 16:54:23 2015 +0100
[AArch64] Handle -|x| case using a single csneg
diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md
index e6d0764..6664d1a 100644
--- a/gcc/config/aarch64/aarch64.md
+++ b/gcc/config/aarch64/aarch64.md
@@ -2333,6 +2333,29 @@ (define_expand "abs<mode>2"
}
)
+;; Combine will try merging (c > 0 ? -x : x) into (-|x|). This isn't a good
+;; idea if the target has a conditional negate instruction and no integer
+;; abs instruction, but the midend doesn't have an optab for conditional neg
+;; and we advertise an optab for abs, so match that case here and emit the
+;; optimal CSNEG variant.
+(define_insn_and_split "*absneg2<mode>_insn"
+ [(set (match_operand:GPI 0 "register_operand" "=r")
+ (neg:GPI
+ (abs:GPI (match_operand:GPI 1 "register_operand" "r"))))]
+ ""
+ "#"
+ ""
+ [(const_int 0)]
+ {
+ rtx ccreg = aarch64_gen_compare_reg (LT, operands[1], const0_rtx);
+ rtx x = gen_rtx_GE (VOIDmode, ccreg, const0_rtx);
+ emit_insn (gen_csneg3<mode>_insn (operands[0], x, operands[1],
+ operands[1]));
+ DONE;
+ }
+ [(set_attr "type" "csel")]
+)
+
(define_insn "neg<mode>2"
[(set (match_operand:GPI 0 "register_operand" "=r,w")
(neg:GPI (match_operand:GPI 1 "register_operand" "r,w")))]
diff --git a/gcc/testsuite/gcc.target/aarch64/neg-abs_1.c b/gcc/testsuite/gcc.target/aarch64/neg-abs_1.c
new file mode 100644
index 0000000..cb2a387
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/neg-abs_1.c
@@ -0,0 +1,17 @@
+/* { dg-do compile } */
+/* { dg-options "-save-temps -O2" } */
+
+int
+f1 (int x)
+{
+ return x < 0 ? x : -x;
+}
+
+long long
+f2 (long long x)
+{
+ return x < 0 ? x : -x;
+}
+
+/* { dg-final { scan-assembler-not "\tneg\tw\[0-9\]*.*" } } */
+/* { dg-final { scan-assembler-not "\tneg\tx\[0-9\]*.*" } } */
Hi all, For the testcase in the patch we were generating an extra neg instruction: cmp w0, wzr csneg w0, w0, w0, ge neg w0, w0 ret instead of the optimal: cmp w0, wzr csneg w0, w0, w0, lt ret The reason is that combine tries to merge the operation into a negation of an abs. I considered teaching combine not to do that but it would require telling it that it shouldn't do it if there is a conditional negate instruction. There's no optab for that though :( Also, we already advertise that we have an abs optab, even though we expand to a compare and a csneg anyway. This patch was the cleanest way I could do this. We just match the neg of an abs and generate the same csneg sequence as for normal abs, just with the comparison condition inverted. Bootstrapped and tested on aarch64. Ok for trunk? Thanks, Kyrill 2015-07-13 Kyrylo Tkachov <kyrylo.tkachov@arm.com> * config/aarch64/aarch64.md (*absneg2<mode>_insn): New define_and_split. 2015-07-13 Kyrylo Tkachov <kyrylo.tkachov@arm.com> * gcc.target/aarch64/neg-abs_1.c: New test.