Re: [PATCH][ARM][2/3] Make if_neg_move and if_move_neg into insn_and_split

[Kyrill Tkachov <kyrylo dot tkachov at arm dot com>]

On 31/07/15 11:18, Ramana Radhakrishnan wrote:
> Hmmm, not sure if this is that straightforward just looking at this..
>
> > 2015-07-24  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
> >
> >      * config/arm/arm.md (*if_neg_move): Convert to insn_and_split.
> >      Enable for TARGET_32BIT.
> >      (*if_move_neg): Likewise.
> > commit 1b495b6cb68c77f628e1c1d672c06dcdf5ccf79b
> > Author: Kyrylo Tkachov <kyrylo.tkachov@arm.com>
> > Date:   Thu Jul 23 09:20:41 2015 +0100
> >      [ARM] Make if_neg_move  and if_move_neg into insn_and_split
> > diff --git a/gcc/config/arm/arm.md b/gcc/config/arm/arm.md
> > index 0be70a8..f341109 100644
> > --- a/gcc/config/arm/arm.md
> > +++ b/gcc/config/arm/arm.md
> > @@ -10064,21 +10064,24 @@ (define_insn "*ifcompare_neg_move"
> >      (set_attr "type" "multiple")]
> >   )
> >   
> > -(define_insn "*if_neg_move"
> > -  [(set (match_operand:SI 0 "s_register_operand" "=r,r,r")
> > +(define_insn_and_split "*if_neg_move"
> > +  [(set (match_operand:SI 0 "s_register_operand" "=l,r")
> >   	(if_then_else:SI
> >   	 (match_operator 4 "arm_comparison_operator"
> >   	  [(match_operand 3 "cc_register" "") (const_int 0)])
> > -	 (neg:SI (match_operand:SI 2 "s_register_operand" "r,r,r"))
> > -	 (match_operand:SI 1 "arm_not_operand" "0,?rI,K")))]
> > -  "TARGET_ARM"
> > -  "@
> > -   rsb%d4\\t%0, %2, #0
> > -   mov%D4\\t%0, %1\;rsb%d4\\t%0, %2, #0
> > -   mvn%D4\\t%0, #%B1\;rsb%d4\\t%0, %2, #0"
> > +	 (neg:SI (match_operand:SI 2 "s_register_operand" "l,r"))
> > +	 (match_operand:SI 1 "arm_not_operand" "0,0")))]
>
>
> So, we have a predicate that doesn't cover all the constraints - in this case aren't we forcing everything into operand0. What happens if we just delete this pattern instead of turning it into an insn_and_split - after all we have other parts of the backend where conditional negates and conditional moves will be caught and cond-exec probably post dates some of these if-then-else patterns.

Hmmm yes, I think operand 1 should be tightened to s_register_operand.
The reason I want this pattern is so that I can expand to it in patch 3/3 where I want to create
a conditional negate expression. However, I can't just emit a COND_EXEC at expand time. I found that
reload doesn't handle the dataflow through them properly. With this pattern I can carry the if_then_else
around and split it into the conditional negate only after reload when it's safe.

Kyrill

> > +  "TARGET_32BIT"
> > +  "#"
> > +  "&& reload_completed"
> > +  [(cond_exec (match_op_dup 4 [(match_dup 3) (const_int 0)])
> > +	      (set (match_dup 0) (neg:SI (match_dup 2))))]
> > +  ""
> >     [(set_attr "conds" "use")
> > -   (set_attr "length" "4,8,8")
> > -   (set_attr "type" "logic_shift_imm,multiple,multiple")]
> > +   (set_attr "length" "4")
> > +   (set_attr "arch" "t2,32")
> > +   (set_attr "enabled_for_depr_it" "yes,no")
> > +   (set_attr "type" "logic_shift_imm")]
> >   )
> >   
> >   (define_insn "*ifcompare_move_neg"
> > @@ -10097,21 +10100,34 @@ (define_insn "*ifcompare_move_neg"
> >      (set_attr "type" "multiple")]
> >   )
> >   
> > -(define_insn "*if_move_neg"
> > -  [(set (match_operand:SI 0 "s_register_operand" "=r,r,r")
> > +(define_insn_and_split "*if_move_neg"
> > +  [(set (match_operand:SI 0 "s_register_operand" "=l,r")
> >   	(if_then_else:SI
> >   	 (match_operator 4 "arm_comparison_operator"
> >   	  [(match_operand 3 "cc_register" "") (const_int 0)])
> > -	 (match_operand:SI 1 "arm_not_operand" "0,?rI,K")
> > -	 (neg:SI (match_operand:SI 2 "s_register_operand" "r,r,r"))))]
> > -  "TARGET_ARM"
> > -  "@
> > -   rsb%D4\\t%0, %2, #0
> > -   mov%d4\\t%0, %1\;rsb%D4\\t%0, %2, #0
> > -   mvn%d4\\t%0, #%B1\;rsb%D4\\t%0, %2, #0"
> > +	 (match_operand:SI 1 "arm_not_operand" "0,0")
> > +	 (neg:SI (match_operand:SI 2 "s_register_operand" "l,r"))))]
> > +  "TARGET_32BIT"
> > +  "#"
> > +  "&& reload_completed"
> > +  [(cond_exec (match_dup 5)
> > +	      (set (match_dup 0) (neg:SI (match_dup 2))))]
> > +  {
> > +    machine_mode mode = GET_MODE (operands[3]);
> > +    rtx_code rc = GET_CODE (operands[4]);
> > +
> > +    if (mode == CCFPmode || mode == CCFPEmode)
> > +      rc = reverse_condition_maybe_unordered (rc);
> > +    else
> > +      rc = reverse_condition (rc);
> > +
> > +    operands[5] = gen_rtx_fmt_ee (rc, VOIDmode, operands[3], const0_rtx);
> > +  }
> >     [(set_attr "conds" "use")
> > -   (set_attr "length" "4,8,8")
> > -   (set_attr "type" "logic_shift_imm,multiple,multiple")]
> > +   (set_attr "length" "4")
> > +   (set_attr "arch" "t2,32")
> > +   (set_attr "enabled_for_depr_it" "yes,no")
> > +   (set_attr "type" "logic_shift_imm")]
> >   )
>
> Same as above.
>
> >   
> >   (define_insn "*arith_adjacentmem"