Thanks for the patch and sorry for the slow reply.
Must admit that I hadn't realised that we'd quite that many
autodetect_types, sorry. Obviously the operand numbering is a lot
less regular in arm than in aarch64. :-) The approach still seems
reasonable to me though, and the patch generally looks really good.
Qian Jianhua <qia...@cn.fujitsu.com> writes:
> diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md
> index dbc6b1db176..12418f42ee5 100644
> --- a/gcc/config/aarch64/aarch64.md
> +++ b/gcc/config/aarch64/aarch64.md
> @@ -2447,7 +2447,7 @@
> (match_operand:GPI 3 "register_operand" "r")))]
> ""
> "add\\t%<w>0, %<w>3, %<w>1, <shift> %2"
> - [(set_attr "type" "alu_shift_imm")]
> + [(set_attr "autodetect_type" "alu_shift_operator")]
> )
The full pattern is:
(define_insn "*add_<shift>_<mode>"
[(set (match_operand:GPI 0 "register_operand" "=r")
(plus:GPI (ASHIFT:GPI (match_operand:GPI 1 "register_operand" "r")
(match_operand:QI 2 "aarch64_shift_imm_<mode>"
"n"))
(match_operand:GPI 3 "register_operand" "r")))]
""
"add\\t%<w>0, %<w>3, %<w>1, <shift> %2"
[(set_attr "autodetect_type" "alu_shift_operator")]
)
so I think in this case it would be better to have:
alu_shift_<shift>_op2
and define alu_shift_lsr_op2 and alu_shift_asr_op2 autodetect_types that
always map to alu_shift_imm_other.
I think all of the aarch64.md uses would then also be:
alu_shift_<shift>_op2
> @@ -1370,7 +1371,8 @@
> (set_attr "arch" "32,a")
> (set_attr "shift" "3")
> (set_attr "predicable" "yes")
> - (set_attr "type" "alu_shift_imm,alu_shift_reg")]
> + (set_attr "autodetect_type" "alu_shift_operator2,none")
> + (set_attr "type" "*,alu_shift_reg")]
> )
>
> (define_insn "*addsi3_carryin_clobercc"
I guess here we have the option of using just:
(set_attr "autodetect_type" "alu_shift_operator2")
We can then make alu_shift_operator2 detect shifts by registers too.
It looked like this could simplify some of the other patterns too.
Neither way's obviously better than the other, just mentioning it
as a suggestion.
> @@ -9501,7 +9509,7 @@
> [(set_attr "predicable" "yes")
> (set_attr "shift" "2")
> (set_attr "arch" "a,t2")
> - (set_attr "type" "alu_shift_imm")])
> + (set_attr "autodetect_type" "alu_shift_lsl_op3")])
The pattern here is:
(define_insn "*<arith_shift_insn>_multsi"
[(set (match_operand:SI 0 "s_register_operand" "=r,r")
(SHIFTABLE_OPS:SI
(mult:SI (match_operand:SI 2 "s_register_operand" "r,r")
(match_operand:SI 3 "power_of_two_operand" ""))
(match_operand:SI 1 "s_register_operand" "rk,<t2_binop0>")))]
"TARGET_32BIT"
"<arith_shift_insn>%?\\t%0, %1, %2, lsl %b3"
[(set_attr "predicable" "yes")
(set_attr "shift" "2")
(set_attr "arch" "a,t2")
(set_attr "autodetect_type" "alu_shift_lsl_op3")])
so I think alu_shift_mul_op3 would be a better name.
(By rights this pattern should never match, since the mult should
be converted to a shift. But fixing that would be feature creep. :-))
> diff --git a/gcc/config/arm/common.md b/gcc/config/arm/common.md
> new file mode 100644
> index 00000000000..1a5da834d61
> --- /dev/null
> +++ b/gcc/config/arm/common.md
> @@ -0,0 +1,37 @@
> +;; Common predicate definitions for ARM, Thumb and AArch64
> +;; Copyright (C) 2020 Free Software Foundation, Inc.
> +;; Contributed by Fujitsu Ltd.
> +
> +;; This file is part of GCC.
> +
> +;; GCC is free software; you can redistribute it and/or modify it
> +;; under the terms of the GNU General Public License as published
> +;; by the Free Software Foundation; either version 3, or (at your
> +;; option) any later version.
> +
> +;; GCC is distributed in the hope that it will be useful, but WITHOUT
> +;; ANY WARRANTY; without even the implied warranty of MERCHANTABILITY
> +;; or FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public
> +;; License for more details.
> +
> +;; You should have received a copy of the GNU General Public License
> +;; along with GCC; see the file COPYING3. If not see
> +;; <http://www.gnu.org/licenses/>.
> +
> +;; Return true if constant is CONST_INT >= 1 and <= 4
> +(define_predicate "const_1_to_4_operand"
> + (and (match_code "const_int")
> + (match_test "IN_RANGE(INTVAL (op), 1, 4)")))
Minor formatting nit, but: GCC style is to have a space between
"IN_RANGE" and "(".
> +;; Return true if constant is 2 or 4 or 8 or 16
> +(define_predicate "const_2_4_8_16_operand"
> + (and (match_code "const_int")
> + (match_test (" INTVAL (op) == 2
> + || INTVAL (op) == 4
> + || INTVAL (op) == 8
> + || INTVAL (op) == 16 "))))
> +
> +;; Return true if shift type is lsl and amount is in[1,4].
> +(define_predicate "alu_shift_operator_lsl_1_to_4"
> + (and (match_code "ashift")
> + (match_test "const_1_to_4_operand(XEXP(op, 1), mode)")))
Same space comment here.
> diff --git a/gcc/config/arm/types.md b/gcc/config/arm/types.md
> index 83983452f52..d1303c6fd76 100644
> --- a/gcc/config/arm/types.md
> +++ b/gcc/config/arm/types.md
> @@ -19,6 +19,20 @@
> ;; along with GCC; see the file COPYING3. If not see
> ;; <http://www.gnu.org/licenses/>.
>
> +; The insn need to autodetect for specific type attribute
> +(define_attr "autodetect_type"
> + "none,\
> + alu_shift_lsl_op2,\
> + alu_shift_lsl_op3,\
> + alu_shift_operator,\
> + alu_shift_operator_zextend,\
> + alu_shift_operator1,\
> + alu_shift_operator2,\
> + alu_shift_operator3,\
> + alu_shift_operator4,\
> + alu_shift_operator5"
I don't think the patch uses alu_shift_operator5.
I realise you're just following existing style here, but the backslashes
shouldn't be needed. (Let me know if that turns out not to be true.)
> @@ -39,16 +53,22 @@
> ; or an immediate operand. This excludes
> ; MOV and MVN but includes MOVT. This also excludes
> ; DSP-kind instructions. This is also the default.
> -; alu_shift_imm any arithmetic instruction that has a source operand
> +; alu_shift_imm_lsl_1to4
> +; any arithmetic instruction that has a source operand
> ; shifted by a constant. This excludes simple shifts.
Maybe adjust this to "shifted left by a constant in the range 1 to 4".
> -; alu_shift_reg as alu_shift_imm, with the shift amount specified in a
> +; The shift type is LSL. And the shift amount is greater
> +; than or equal 1, and less than or equal 4.
> +; alu_shift_imm_other
> +; as alu_shift_imm_lsl_1to4, with she shift type is LSR or
s/she/the/
Thanks,
Richard
