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

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

[Kyrill Tkachov]

[-- Attachment #1: Type: text/plain, Size: 1174 bytes --]

Hi all,

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.
I considered teaching combine not to do that but it would require telling it that it shouldn't
do it if there is a conditional negate instruction. There's no optab for that though :(
Also, we already advertise that we have an abs optab, even though we expand to a compare and
a csneg anyway. This patch was the cleanest way I could do this. We just match the neg of an abs
and generate the same csneg sequence as for normal abs, just with the comparison condition inverted.

Bootstrapped and tested on aarch64.

Ok for trunk?
Thanks,
Kyrill

2015-07-13  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>

     * config/aarch64/aarch64.md (*absneg2<mode>_insn): New
     define_and_split.

2015-07-13  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>

     * gcc.target/aarch64/neg-abs_1.c: New test.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: abs-neg.patch --]
[-- Type: text/x-patch; name=abs-neg.patch, Size: 1998 bytes --]

commit 7527a76d25067ce4a5426e563e162487604ac6c1
Author: Kyrylo Tkachov <kyrylo.tkachov@arm.com>
Date:   Thu Jul 9 16:54:23 2015 +0100

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

diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md
index e6d0764..6664d1a 100644
--- a/gcc/config/aarch64/aarch64.md
+++ b/gcc/config/aarch64/aarch64.md
@@ -2333,6 +2333,29 @@ (define_expand "abs<mode>2"
   }
 )
 
+;; Combine will try merging (c > 0 ? -x : x) into (-|x|).  This isn't a good
+;; idea if the target has a conditional negate instruction and no integer
+;; abs instruction, but the midend doesn't have an optab for conditional neg
+;; and we advertise an optab for abs, so match that case here and emit the
+;; optimal CSNEG variant.
+(define_insn_and_split "*absneg2<mode>_insn"
+  [(set (match_operand:GPI 0 "register_operand" "=r")
+	(neg:GPI
+	  (abs:GPI (match_operand:GPI 1 "register_operand" "r"))))]
+  ""
+  "#"
+  ""
+  [(const_int 0)]
+  {
+    rtx ccreg = aarch64_gen_compare_reg (LT, operands[1], const0_rtx);
+    rtx x = gen_rtx_GE (VOIDmode, ccreg, const0_rtx);
+    emit_insn (gen_csneg3<mode>_insn (operands[0], x, operands[1],
+				       operands[1]));
+    DONE;
+  }
+  [(set_attr "type" "csel")]
+)
+
 (define_insn "neg<mode>2"
   [(set (match_operand:GPI 0 "register_operand" "=r,w")
 	(neg:GPI (match_operand:GPI 1 "register_operand" "r,w")))]
diff --git a/gcc/testsuite/gcc.target/aarch64/neg-abs_1.c b/gcc/testsuite/gcc.target/aarch64/neg-abs_1.c
new file mode 100644
index 0000000..cb2a387
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/neg-abs_1.c
@@ -0,0 +1,17 @@
+/* { dg-do compile } */
+/* { dg-options "-save-temps -O2" } */
+
+int
+f1 (int x)
+{
+  return x < 0 ? x : -x;
+}
+
+long long
+f2 (long long x)
+{
+  return x < 0 ? x : -x;
+}
+
+/* { dg-final { scan-assembler-not "\tneg\tw\[0-9\]*.*" } } */
+/* { dg-final { scan-assembler-not "\tneg\tx\[0-9\]*.*" } } */

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

[Segher Boessenkool]

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

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.


Segher

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

[Kyrill Tkachov]

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


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
>

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

[Andrew Pinski]

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.

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

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

[Andrew Pinski]

On Tue, Jul 14, 2015 at 3:06 AM, Andrew Pinski <pinskia@gmail.com> 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.

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

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

[Kyrill Tkachov]

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?

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

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

[Andrew Pinski]

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

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

[Kyrill Tkachov]

On 14/07/15 11:40, Andrew Pinski wrote:
> 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.

Ok, I dug a bit further. There's a couple of issues here:
* combine_simplify_rtx tries to simplify the comparison in the if_then_else
then has the lines:
        if (cond_code == NE && COMPARISON_P (cond))
          return x;

which quits early and doesn't try optimising the (neg (if_then_else)) further.

* If I bypass that condition it proceeds further into simplify_unary_operation from
simplify-rtx.c which doesn't have the simplifcation rule we want.
If I add a [- (x ? -y : y) -> !x ? -y : y] transformation there then it works.

I'll try to whip a patch into shape.

Thanks,
Kyrill

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

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

[Segher Boessenkool]

On Tue, Jul 14, 2015 at 09:18:06AM +0100, Kyrill Tkachov wrote:
> >Before combine, you have two insns, a negation and an abs, so that is
> >not so very strange :-)

Oh, hrm, my aarch64 cross was three months old, and this now changed.
Or I messed up.  Sorry for the noise.

It does look like the if_then_else stuff in combine (and simplify-rtx)
could use some improvement, probably more than just fixing this one
case.  Or look at the first huge case in combine_simplify_rtx, that
looks eerily similar.


Segher

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

[Kyrill Tkachov]

On 14/07/15 13:55, Segher Boessenkool wrote:
> On Tue, Jul 14, 2015 at 09:18:06AM +0100, Kyrill Tkachov wrote:
>>> Before combine, you have two insns, a negation and an abs, so that is
>>> not so very strange :-)
> Oh, hrm, my aarch64 cross was three months old, and this now changed.
> Or I messed up.  Sorry for the noise.
>
> It does look like the if_then_else stuff in combine (and simplify-rtx)
> could use some improvement, probably more than just fixing this one
> case.  Or look at the first huge case in combine_simplify_rtx, that
> looks eerily similar.

Yeah, removing an early return in combine_simplify_rtx and
adding the simplification to simplify_unary_operation did
the trick.

I'll see if it survives testing.

Thanks,
Kyrill

>
>
> Segher
>

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

[pinskia]

> On Jul 13, 2015, at 5:48 PM, Kyrill Tkachov <kyrylo.tkachov@arm.com> wrote:
> 
> Hi all,
> 
> 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.
> I considered teaching combine not to do that but it would require telling it that it shouldn't
> do it if there is a conditional negate instruction. There's no optab for that though :(
> Also, we already advertise that we have an abs optab, even though we expand to a compare and
> a csneg anyway. This patch was the cleanest way I could do this. We just match the neg of an abs
> and generate the same csneg sequence as for normal abs, just with the comparison condition inverted.
> 
> Bootstrapped and tested on aarch64.
> 
> Ok for trunk?
> Thanks,
> Kyrill
> 
> 2015-07-13  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
> 
>    * config/aarch64/aarch64.md (*absneg2<mode>_insn): New
>    define_and_split.

This pattern is incorrect as you need to say you are clobbering the flags register. Otherwise an optimization between combine and the splitter can move an instruction between it. Also it might be better to just have a define_split rather than a define_insn_and_split. Combine knows how to use define_split without being an insn. 

Thanks,
Andrew

> 
> 2015-07-13  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
> 
>    * gcc.target/aarch64/neg-abs_1.c: New test.
> <abs-neg.patch>

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

[Segher Boessenkool]

On Tue, Jul 14, 2015 at 07:04:13PM +0800, pinskia@gmail.com wrote:
> Combine knows how to use define_split without being an insn. 

Combine uses define_split in very different circumstances than it uses
define_insn.  In this case, define_split will only do anything if the
"nabs" is combined from three insns, while a define_insn handles it
being combined from two as well.

A define_split will be split during combine (and combine can then work
on its factors); a define_insn is only split _after_ combine.


Segher