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