Hi Oleg,
On 22/09/15 14:35, Oleg Endo wrote:
On SH, the result of comparisons etc. is stored in the T_REG. It's a 1
bit reg but described as SImode. To get the T_REG into another reg,
there's this insn:
(define_insn "movt"
[(set (match_operand:SI 0 "arith_reg_dest" "=r")
(match_operand:SI 1 "t_reg_operand"))]
"TARGET_SH1"
"movt %0"
[(set_attr "type" "arith")])
where "t_reg_operand" accepts various forms of the T_REG via
reg,subreg,sign_extend,zero_extend (to get better combine results).
Now I'd like to extend the "t_reg_operand" predicate to accept something
like
(set (reg:SI) (ne:SI (reg:SI 147 t) (const_int 0)))
For the test case
int test05 (int a, int b)
{
return a != b ? 0x7FFFFFFF : 0x80000000;
}
ifcvt (noce_emit_store_flag) tries to emit the store flag insn as
(set (reg:SI 162)
(ne:SI (reg:SI 147 t)
(const_int 0 [0])))
It then proceeds with a call to
expand_simple_binop
op0: (const_int 2147483647 [0x7fffffff])
op1: (reg:SI 162) <<< same reg
target: (reg:SI 162) <<<
where does noce_emit_store_flag call expand_simple_binop?
Do you mean the code following the call to noce_emit_store_flag
in noce_try_store_flag_constants? (I suspect that's the code that
will get triggered for your testcase)
The problem is that expand_binop gets confused, seemingly because
op1 and target are the same. It tries to emit a 64 bit addition:
(insn 50 49 51 (clobber (reg:DI 173)) -1
(nil))
(insn 51 50 52 (set (subreg:SI (reg:DI 173) 0)
(reg:SI 162)) -1
(nil))
(insn 52 51 53 (set (reg:DI 175)
(const_int 2147483647 [0x7fffffff])) -1
(nil))
(insn 53 52 0 (parallel [
(set (reg:DI 174)
(plus:DI (reg:DI 173)
(reg:DI 175)))
(clobber (reg:SI 147 t))
]) -1
(nil))
huh, why does it try to do that? I'll admit I don't know the details
of how that expansion works. Presumably it's very target-specific
ifcvt (end_ifcvt_sequence) then does a recog for each emitted insn in
the resulting sequence and insn 50 above will not match anything in the
SH md. It then drops everything and tries something else.
Until now, because SH patterns would not accept the
(set (reg:SI 162)
(ne:SI (reg:SI 147 t)
(const_int 0 [0])))
the "fallback path" in noce_emit_store_flag is used. This uses
emit_store_flag which creates a new pseudo for the result and we get
expand_simple_binop
op0: (const_int 2147483647 [0x7fffffff])
op1: (reg:SI 167) <<< different regs
target: (reg:SI 162) <<<
which works fine (no 64 bit addition).
A simple fix seems to be to create a new pseudo for the "short cut" path
in noce_emit_store_flag:
Index: gcc/ifcvt.c
===================================================================
--- gcc/ifcvt.c (revision 227970)
+++ gcc/ifcvt.c (working copy)
@@ -874,6 +874,7 @@
{
rtx src = gen_rtx_fmt_ee (code, GET_MODE (x), XEXP (cond, 0),
XEXP (cond, 1));
+ x = gen_reg_rtx (GET_MODE (x));
rtx set = gen_rtx_SET (x, src);
start_sequence ();
I haven't done full testing of this, but it fixes the problem for me.
Any opinions?
Looks reasonable to me with one caveat.
The comment text for noce_emit_store_flag is not very helpful
and my first expectation of the X parameter to it is that it is the
target where the flag is stored. With your case this will no longer
be the case (emit_store_flag seems to attempt to return the result in
x, judging by its comment in expmed.c). I think it would be a good idea
to expand the comment of noce_emit_store_flag to describe its arguments
and to mention that the result is returned but is not necessarily placed in X.
I think all the callers in ifcvt.c already guard against that possibility and
do something along the lines of:
target = noce_emit_store_flag (..., x, ..., ...);
if (target != if_info->x)
noce_emit_move_insn (if_info->x, target);
but it would be nice to go through the callers and make sure that they don't
assume
that the result of noce_emit_store_flag is in always x.
Kyrill
Cheers,
Oleg
