RFA: cache enabled attribute by insn code

Richard Sandiford <rdsandiford@googlemail.com> writes:
> Richard Sandiford <rsandifo@linux.vnet.ibm.com> writes:
>> Does the following patch help?
>
> Bah, it won't of course: %i1 needs to be the operator.

Here's v2.  I tested that it worked for simple tests like:

int f1 (int x, int y) { return x + (y << 4); }
int f2 (int x, int y) { return x - (y << 4); }
int f3 (int x, int y) { return x & (y << 4); }
int f4 (int x, int y) { return x | (y << 4); }
int f5 (int x, int y) { return x ^ (y << 4); }
int f6 (int x, int y) { return (y << 4) - x; }
int g1 (int x, int y, int z) { return x + (y << z); }
int g2 (int x, int y, int z) { return x - (y << z); }
int g3 (int x, int y, int z) { return x & (y << z); }
int g4 (int x, int y, int z) { return x | (y << z); }
int g5 (int x, int y, int z) { return x ^ (y << z); }
int g6 (int x, int y, int z) { return (y << z) - x; }

as well as the testcase.

Thanks,
Richard


gcc/
	* config/arm/iterators.md (shiftable_ops): New code iterator.
	(t2_binop0, arith_shift_insn): New code attributes.
	* config/arm/arm.md (insn_enabled): Delete.
	(enabled): Remove insn_enabled test.
	(*arith_shiftsi): Split out...
	(*arith_multsi): ...this pattern and remove insn_enabled attribute.

On 27 May 2014 17:07, Richard Sandiford <rdsandiford@googlemail.com> wrote:
> Richard Sandiford <rdsandiford@googlemail.com> writes:
>> Richard Sandiford <rsandifo@linux.vnet.ibm.com> writes:
>>> Does the following patch help?
>>
>> Bah, it won't of course: %i1 needs to be the operator.
>
> Here's v2.  I tested that it worked for simple tests like:
>

I confirm that the compiler now builds fine (target
arm-none-linux-gnueabihf) with this patch applied.

Thanks,

Christophe.

> int f1 (int x, int y) { return x + (y << 4); }
> int f2 (int x, int y) { return x - (y << 4); }
> int f3 (int x, int y) { return x & (y << 4); }
> int f4 (int x, int y) { return x | (y << 4); }
> int f5 (int x, int y) { return x ^ (y << 4); }
> int f6 (int x, int y) { return (y << 4) - x; }
> int g1 (int x, int y, int z) { return x + (y << z); }
> int g2 (int x, int y, int z) { return x - (y << z); }
> int g3 (int x, int y, int z) { return x & (y << z); }
> int g4 (int x, int y, int z) { return x | (y << z); }
> int g5 (int x, int y, int z) { return x ^ (y << z); }
> int g6 (int x, int y, int z) { return (y << z) - x; }
>
> as well as the testcase.
>
> Thanks,
> Richard
>
>
> gcc/
>         * config/arm/iterators.md (shiftable_ops): New code iterator.
>         (t2_binop0, arith_shift_insn): New code attributes.
>         * config/arm/arm.md (insn_enabled): Delete.
>         (enabled): Remove insn_enabled test.
>         (*arith_shiftsi): Split out...
>         (*arith_multsi): ...this pattern and remove insn_enabled attribute.
>
> Index: gcc/config/arm/arm.md
> ===================================================================
> --- gcc/config/arm/arm.md       (revision 210972)
> +++ gcc/config/arm/arm.md       (working copy)
> @@ -200,17 +200,9 @@
>           (const_string "yes")]
>          (const_string "no")))
>
> -; Allows an insn to disable certain alternatives for reasons other than
> -; arch support.
> -(define_attr "insn_enabled" "no,yes"
> -  (const_string "yes"))
> -
>  ; Enable all alternatives that are both arch_enabled and insn_enabled.
>   (define_attr "enabled" "no,yes"
> -   (cond [(eq_attr "insn_enabled" "no")
> -         (const_string "no")
> -
> -         (and (eq_attr "predicable_short_it" "no")
> +   (cond [(and (eq_attr "predicable_short_it" "no")
>                (and (eq_attr "predicated" "yes")
>                     (match_test "arm_restrict_it")))
>           (const_string "no")
> @@ -9876,39 +9868,38 @@
>
>  ;; Patterns to allow combination of arithmetic, cond code and shifts
>
> -(define_insn "*arith_shiftsi"
> -  [(set (match_operand:SI 0 "s_register_operand" "=r,r,r,r")
> -        (match_operator:SI 1 "shiftable_operator"
> -          [(match_operator:SI 3 "shift_operator"
> -             [(match_operand:SI 4 "s_register_operand" "r,r,r,r")
> -              (match_operand:SI 5 "shift_amount_operand" "M,M,M,r")])
> -           (match_operand:SI 2 "s_register_operand" "rk,rk,r,rk")]))]
> +;; Separate out the (mult ...) case, since it doesn't allow register operands.
> +(define_insn "*arith_multsi"
> +  [(set (match_operand:SI 0 "s_register_operand" "=r,r")
> +        (shiftable_ops:SI
> +         (match_operator:SI 2 "mult_operator"
> +           [(match_operand:SI 3 "s_register_operand" "r,r")
> +            (match_operand:SI 4 "power_of_two_operand")])
> +         (match_operand:SI 1 "s_register_operand" "rk,<t2_binop0>")))]
>    "TARGET_32BIT"
> -  "%i1%?\\t%0, %2, %4%S3"
> +  "<arith_shift_insn>%?\\t%0, %1, %3%S2"
>    [(set_attr "predicable" "yes")
>     (set_attr "predicable_short_it" "no")
>     (set_attr "shift" "4")
> -   (set_attr "arch" "a,t2,t2,a")
> -   ;; Thumb2 doesn't allow the stack pointer to be used for
> -   ;; operand1 for all operations other than add and sub. In this case
> -   ;; the minus operation is a candidate for an rsub and hence needs
> -   ;; to be disabled.
> -   ;; We have to make sure to disable the fourth alternative if
> -   ;; the shift_operator is MULT, since otherwise the insn will
> -   ;; also match a multiply_accumulate pattern and validate_change
> -   ;; will allow a replacement of the constant with a register
> -   ;; despite the checks done in shift_operator.
> -   (set_attr_alternative "insn_enabled"
> -                        [(const_string "yes")
> -                         (if_then_else
> -                          (match_operand:SI 1 "add_operator" "")
> -                          (const_string "yes") (const_string "no"))
> -                         (const_string "yes")
> -                         (if_then_else
> -                          (match_operand:SI 3 "mult_operator" "")
> -                          (const_string "no") (const_string "yes"))])
> -   (set_attr "type" "alu_shift_imm,alu_shift_imm,alu_shift_imm,alu_shift_reg")])
> +   (set_attr "arch" "a,t2")
> +   (set_attr "type" "alu_shift_imm,alu_shift_imm")])
>
> +;; Handles cases other than the above.
> +(define_insn "*arith_shiftsi"
> +  [(set (match_operand:SI 0 "s_register_operand" "=r,r,r")
> +        (shiftable_ops:SI
> +          (match_operator:SI 2 "shift_operator"
> +            [(match_operand:SI 3 "s_register_operand" "r,r,r")
> +             (match_operand:SI 4 "shift_amount_operand" "M,M,r")])
> +          (match_operand:SI 1 "s_register_operand" "rk,<t2_binop0>,rk")))]
> +  "TARGET_32BIT && GET_CODE (operands[3]) != MULT"
> +  "<arith_shift_insn>%?\\t%0, %1, %3%S2"
> +  [(set_attr "predicable" "yes")
> +   (set_attr "predicable_short_it" "no")
> +   (set_attr "shift" "4")
> +   (set_attr "arch" "a,t2,a")
> +   (set_attr "type" "alu_shift_imm,alu_shift_imm,alu_shift_reg")])
> +
>  (define_split
>    [(set (match_operand:SI 0 "s_register_operand" "")
>         (match_operator:SI 1 "shiftable_operator"
> Index: gcc/config/arm/iterators.md
> ===================================================================
> --- gcc/config/arm/iterators.md (revision 210972)
> +++ gcc/config/arm/iterators.md (working copy)
> @@ -194,6 +194,20 @@
>  ;; Right shifts
>  (define_code_iterator rshifts [ashiftrt lshiftrt])
>
> +;; Binary operators whose second operand can be shifted.
> +(define_code_iterator shiftable_ops [plus minus ior xor and])
> +
> +;; plus and minus are the only shiftable_ops for which Thumb2 allows
> +;; a stack pointer opoerand.  The minus operation is a candidate for an rsub
> +;; and hence only plus is supported.
> +(define_code_attr t2_binop0
> +  [(plus "rk") (minus "r") (ior "r") (xor "r") (and "r")])
> +
> +;; The instruction to use when a shiftable_ops has a shift operation as
> +;; its first operand.
> +(define_code_attr arith_shift_insn
> +  [(plus "add") (minus "rsb") (ior "orr") (xor "eor") (and "and")])
> +
>  ;;----------------------------------------------------------------------------
>  ;; Int iterators
>  ;;----------------------------------------------------------------------------

The patch also fixes the arm-none-eabi build failures I've seen.

Thanks,
Yufeng

On 05/27/14 16:07, Richard Sandiford wrote:
> Richard Sandiford<rdsandiford@googlemail.com>  writes:
>> Richard Sandiford<rsandifo@linux.vnet.ibm.com>  writes:
>>> Does the following patch help?
>>
>> Bah, it won't of course: %i1 needs to be the operator.
>
> Here's v2.  I tested that it worked for simple tests like:
>
> int f1 (int x, int y) { return x + (y<<  4); }
> int f2 (int x, int y) { return x - (y<<  4); }
> int f3 (int x, int y) { return x&  (y<<  4); }
> int f4 (int x, int y) { return x | (y<<  4); }
> int f5 (int x, int y) { return x ^ (y<<  4); }
> int f6 (int x, int y) { return (y<<  4) - x; }
> int g1 (int x, int y, int z) { return x + (y<<  z); }
> int g2 (int x, int y, int z) { return x - (y<<  z); }
> int g3 (int x, int y, int z) { return x&  (y<<  z); }
> int g4 (int x, int y, int z) { return x | (y<<  z); }
> int g5 (int x, int y, int z) { return x ^ (y<<  z); }
> int g6 (int x, int y, int z) { return (y<<  z) - x; }
>
> as well as the testcase.
>
> Thanks,
> Richard
>
>
> gcc/
> 	* config/arm/iterators.md (shiftable_ops): New code iterator.
> 	(t2_binop0, arith_shift_insn): New code attributes.
> 	* config/arm/arm.md (insn_enabled): Delete.
> 	(enabled): Remove insn_enabled test.
> 	(*arith_shiftsi): Split out...
> 	(*arith_multsi): ...this pattern and remove insn_enabled attribute.
>
> Index: gcc/config/arm/arm.md
> ===================================================================
> --- gcc/config/arm/arm.md	(revision 210972)
> +++ gcc/config/arm/arm.md	(working copy)
> @@ -200,17 +200,9 @@
>   	  (const_string "yes")]
>   	 (const_string "no")))
>
> -; Allows an insn to disable certain alternatives for reasons other than
> -; arch support.
> -(define_attr "insn_enabled" "no,yes"
> -  (const_string "yes"))
> -
>   ; Enable all alternatives that are both arch_enabled and insn_enabled.
>    (define_attr "enabled" "no,yes"
> -   (cond [(eq_attr "insn_enabled" "no")
> -	  (const_string "no")
> -
> -	  (and (eq_attr "predicable_short_it" "no")
> +   (cond [(and (eq_attr "predicable_short_it" "no")
>   	       (and (eq_attr "predicated" "yes")
>   	            (match_test "arm_restrict_it")))
>   	  (const_string "no")
> @@ -9876,39 +9868,38 @@
>   
>   ;; Patterns to allow combination of arithmetic, cond code and shifts
>
> -(define_insn "*arith_shiftsi"
> -  [(set (match_operand:SI 0 "s_register_operand" "=r,r,r,r")
> -        (match_operator:SI 1 "shiftable_operator"
> -          [(match_operator:SI 3 "shift_operator"
> -             [(match_operand:SI 4 "s_register_operand" "r,r,r,r")
> -              (match_operand:SI 5 "shift_amount_operand" "M,M,M,r")])
> -           (match_operand:SI 2 "s_register_operand" "rk,rk,r,rk")]))]
> +;; Separate out the (mult ...) case, since it doesn't allow register operands.
> +(define_insn "*arith_multsi"
> +  [(set (match_operand:SI 0 "s_register_operand" "=r,r")
> +        (shiftable_ops:SI
> +	  (match_operator:SI 2 "mult_operator"
> +	    [(match_operand:SI 3 "s_register_operand" "r,r")
> +	     (match_operand:SI 4 "power_of_two_operand")])
> +	  (match_operand:SI 1 "s_register_operand" "rk,<t2_binop0>")))]
>     "TARGET_32BIT"
> -  "%i1%?\\t%0, %2, %4%S3"
> +  "<arith_shift_insn>%?\\t%0, %1, %3%S2"
>     [(set_attr "predicable" "yes")
>      (set_attr "predicable_short_it" "no")
>      (set_attr "shift" "4")
> -   (set_attr "arch" "a,t2,t2,a")
> -   ;; Thumb2 doesn't allow the stack pointer to be used for
> -   ;; operand1 for all operations other than add and sub. In this case
> -   ;; the minus operation is a candidate for an rsub and hence needs
> -   ;; to be disabled.
> -   ;; We have to make sure to disable the fourth alternative if
> -   ;; the shift_operator is MULT, since otherwise the insn will
> -   ;; also match a multiply_accumulate pattern and validate_change
> -   ;; will allow a replacement of the constant with a register
> -   ;; despite the checks done in shift_operator.
> -   (set_attr_alternative "insn_enabled"
> -			 [(const_string "yes")
> -			  (if_then_else
> -			   (match_operand:SI 1 "add_operator" "")
> -			   (const_string "yes") (const_string "no"))
> -			  (const_string "yes")
> -			  (if_then_else
> -			   (match_operand:SI 3 "mult_operator" "")
> -			   (const_string "no") (const_string "yes"))])
> -   (set_attr "type" "alu_shift_imm,alu_shift_imm,alu_shift_imm,alu_shift_reg")])
> +   (set_attr "arch" "a,t2")
> +   (set_attr "type" "alu_shift_imm,alu_shift_imm")])
>
> +;; Handles cases other than the above.
> +(define_insn "*arith_shiftsi"
> +  [(set (match_operand:SI 0 "s_register_operand" "=r,r,r")
> +        (shiftable_ops:SI
> +          (match_operator:SI 2 "shift_operator"
> +            [(match_operand:SI 3 "s_register_operand" "r,r,r")
> +             (match_operand:SI 4 "shift_amount_operand" "M,M,r")])
> +          (match_operand:SI 1 "s_register_operand" "rk,<t2_binop0>,rk")))]
> +  "TARGET_32BIT&&  GET_CODE (operands[3]) != MULT"
> +  "<arith_shift_insn>%?\\t%0, %1, %3%S2"
> +  [(set_attr "predicable" "yes")
> +   (set_attr "predicable_short_it" "no")
> +   (set_attr "shift" "4")
> +   (set_attr "arch" "a,t2,a")
> +   (set_attr "type" "alu_shift_imm,alu_shift_imm,alu_shift_reg")])
> +
>   (define_split
>     [(set (match_operand:SI 0 "s_register_operand" "")
>   	(match_operator:SI 1 "shiftable_operator"
> Index: gcc/config/arm/iterators.md
> ===================================================================
> --- gcc/config/arm/iterators.md	(revision 210972)
> +++ gcc/config/arm/iterators.md	(working copy)
> @@ -194,6 +194,20 @@
>   ;; Right shifts
>   (define_code_iterator rshifts [ashiftrt lshiftrt])
>
> +;; Binary operators whose second operand can be shifted.
> +(define_code_iterator shiftable_ops [plus minus ior xor and])
> +
> +;; plus and minus are the only shiftable_ops for which Thumb2 allows
> +;; a stack pointer opoerand.  The minus operation is a candidate for an rsub
> +;; and hence only plus is supported.
> +(define_code_attr t2_binop0
> +  [(plus "rk") (minus "r") (ior "r") (xor "r") (and "r")])
> +
> +;; The instruction to use when a shiftable_ops has a shift operation as
> +;; its first operand.
> +(define_code_attr arith_shift_insn
> +  [(plus "add") (minus "rsb") (ior "orr") (xor "eor") (and "and")])
> +
>   ;;----------------------------------------------------------------------------
>   ;; Int iterators
>   ;;----------------------------------------------------------------------------
>