On Tue, Jul 14, 2015 at 3:13 AM, Kyrill Tkachov <kyrylo.tkac...@arm.com> wrote: > > On 14/07/15 11:06, Andrew Pinski 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. > > > 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 >>>> >