This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH][AArch64] Handle -|x| case using a single csneg
- From: Andrew Pinski <pinskia at gmail dot com>
- To: Kyrill Tkachov <kyrylo dot tkachov at arm dot com>
- Cc: Segher Boessenkool <segher at kernel dot crashing dot org>, GCC Patches <gcc-patches at gcc dot gnu dot org>, Marcus Shawcroft <Marcus dot Shawcroft at arm dot com>, Richard Earnshaw <Richard dot Earnshaw at arm dot com>, James Greenhalgh <James dot Greenhalgh at arm dot com>
- Date: Tue, 14 Jul 2015 03:40:19 -0700
- Subject: Re: [PATCH][AArch64] Handle -|x| case using a single csneg
- Authentication-results: sourceware.org; auth=none
- References: <55A38963 dot 6080408 at arm dot com> <20150714003800 dot GA17453 at gate dot crashing dot org> <55A4C5BE dot 4070602 at arm dot com> <CA+=Sn1n6trq7MS6Y4YtjzMjZp7H0W6n5_P_0V=sHwmWyGzTr=A at mail dot gmail dot com> <55A4E0C1 dot 8040806 at arm 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
>>>>
>