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