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