On 14/07/15 11:40, Andrew Pinski wrote:
On Tue, Jul 14, 2015 at 3:13 AM, Kyrill Tkachov <kyrylo.tkac...@arm.com> wrote:
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?
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
