On Tue, Jul 14, 2015 at 3:06 AM, Andrew Pinski <pins...@gmail.com> wrote: > On Tue, Jul 14, 2015 at 1:18 AM, Kyrill Tkachov <kyrylo.tkac...@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 >>> >>