Hi Richard Thanks for your comments. I have updated the patch.
> -----Original Message----- > From: Richard Sandiford <richard.sandif...@arm.com> > Sent: Thursday, October 1, 2020 3:53 AM > To: Qian, Jianhua/钱 建华 <qia...@cn.fujitsu.com> > Cc: gcc-patches@gcc.gnu.org > Subject: Re: [PATCH] arm&aarch64: subdivide the type attribute > "alu_shfit_imm" > > 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 This is a good idea. Thank you. > > @@ -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. It made the define_insn simply. > > @@ -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. :-)) OK. > > 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. > Modified. > > 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. Exactly. > 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.) Removed the backslashes. > > @@ -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/ Modified. Regards Qian > > Thanks, > Richard >
0001-arm-aarch64-subdivide-the-type-attribute-alu_shfit_i.patch
Description: 0001-arm-aarch64-subdivide-the-type-attribute-alu_shfit_i.patch
