ifcvt vs. expand_binop

Kyrill Tkachov kyrylo.tkachov@arm.com
Tue Sep 22 14:41:00 GMT 2015


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
>



More information about the Gcc-patches mailing list