Re: [PATCH][AArch64] Handle -|x| case using a single csneg

[Andrew Pinski <pinskia at gmail dot com>]

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
>>>>
>