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
