Re: [PATCH] arm: subdivide the type attribute "alu_shfit_imm"

[Richard Sandiford via Gcc-patches]

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  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%0, %3, %1,  %2"
> -  [(set_attr "type" "alu_shift_imm")]
> +  [(set_attr "autodetect_type" "alu_shift_operator")]
>  )

The full pattern is:

(define_insn "*add__"
  [(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_" 
"n"))
  (match_operand:GPI 3 "register_operand" "r")))]
  ""
  "add\\t%0, %3, %1,  %2"
  [(set_attr "autodetect_type" "alu_shift_operator")]
)

so I think in this case it would be better to have:

  alu_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__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 "*_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,")))]
  "TARGET_32BIT"
  "%?\\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 000..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
> +;; .
> +
> +;; 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
> --- 

[PATCH] arm: subdivide the type attribute "alu_shfit_imm"

[Qian Jianhua]

The type attribute "alu_shfit_imm" is subdivided into
"alu_shift_imm_lsl_1to4" and "alu_shift_imm_other", to accommodate
optimazations of some microarchitectures.

Here is the detailed discussion.
https://gcc.gnu.org/pipermail/gcc/2020-September/233594.html

ChangeLog:
2020-09-29 Qian jianhua 

gcc/
* config/arm/types.md (define_attr "autodetect_type"): New.
(define_attr "type"): Subdivide alu_shift_imm.
* config/arm/common.md: New file.
* config/aarch64/predicates.md:Include common.md.
* config/arm/predicates.md:Include common.md.
* config/aarch64/aarch64.md (*add__): Set autodetect_type.
(*add__si_uxtw): Likewise.
(*sub__): Likewise.
(*sub__si_uxtw): Likewise.
(*neg__2): Likewise.
(*neg__si2_uxtw): Likewise.
* config/arm/arm.md (*addsi3_carryin_shift): Likewise.
(add_not_shift_cin): Likewise.
(*subsi3_carryin_shift): Likewise.
(*subsi3_carryin_shift_alt): Likewise.
(*rsbsi3_carryin_shift): Likewise.
(*rsbsi3_carryin_shift_alt): Likewise.
(*arm_shiftsi3): Likewise.
(*_multsi): Likewise.
(*_shiftsi): Likewise.
(subsi3_carryin): Set new type.
(*if_arith_move): Set new type.
(*if_move_arith): Set new type.
(define_attr "core_cycles"): Use new type.
* config/arm/arm-fixed.md (arm_ssatsihi_shift): Set autodetect_type.
* config/arm/thumb2.md (*orsi_not_shiftsi_si): Likewise.
(*thumb2_shiftsi3_short): Set new type.
* config/aarch64/falkor.md (falkor_alu_1_xyz): Use new type.
* config/aarch64/saphira.md (saphira_alu_1_xyz): Likewise.
* config/aarch64/thunderx.md (thunderx_arith_shift): Likewise.
* config/aarch64/thunderx2t99.md (thunderx2t99_alu_shift): Likewise.
* config/aarch64/thunderx3t110.md (thunderx3t110_alu_shift): Likewise.
(thunderx3t110_alu_shift1): Likewise.
* config/aarch64/tsv110.md (tsv110_alu_shift): Likewise.
* config/arm/arm1020e.md (1020alu_shift_op): Likewise.
* config/arm/arm1026ejs.md (alu_shift_op): Likewise.
* config/arm/arm1136jfs.md (11_alu_shift_op): Likewise.
* config/arm/arm926ejs.md (9_alu_op): Likewise.
* config/arm/cortex-a15.md (cortex_a15_alu_shift): Likewise.
* config/arm/cortex-a17.md (cortex_a17_alu_shiftimm): Likewise.
* config/arm/cortex-a5.md (cortex_a5_alu_shift): Likewise.
* config/arm/cortex-a53.md (cortex_a53_alu_shift): Likewise.
* config/arm/cortex-a57.md (cortex_a57_alu_shift): Likewise.
* config/arm/cortex-a7.md (cortex_a7_alu_shift): Likewise.
* config/arm/cortex-a8.md (cortex_a8_alu_shift): Likewise.
* config/arm/cortex-a9.md (cortex_a9_dp_shift): Likewise.
* config/arm/cortex-m4.md (cortex_m4_alu): Likewise.
* config/arm/cortex-m7.md (cortex_m7_alu_shift): Likewise.
* config/arm/cortex-r4.md (cortex_r4_alu_shift): Likewise.
* config/arm/exynos-m1.md (exynos_m1_alu_shift): Likewise.
* config/arm/fa526.md (526_alu_shift_op): Likewise.
* config/arm/fa606te.md (606te_alu_op): Likewise.
* config/arm/fa626te.md (626te_alu_shift_op): Likewise.
* config/arm/fa726te.md (726te_alu_shift_op): Likewise.
* config/arm/fmp626.md (mp626_alu_shift_op): Likewise.
* config/arm/marvell-pj4.md (pj4_shift): Likewise.
(pj4_shift_conds): Likewise.
(pj4_alu_shift): Likewise.
(pj4_alu_shift_conds): Likewise.
* config/arm/xgene1.md (xgene1_alu): Likewise.
* config/arm/arm.c (xscale_sched_adjust_cost): Likewise.


Test Results:
* Bootstrap on aarch64 --- [OK]
* Regression tests on aarch64- [OK]


---
 gcc/config/aarch64/aarch64.md   | 12 ++---
 gcc/config/aarch64/falkor.md|  2 +-
 gcc/config/aarch64/predicates.md|  2 +
 gcc/config/aarch64/saphira.md   |  2 +-
 gcc/config/aarch64/thunderx.md  |  2 +-
 gcc/config/aarch64/thunderx2t99.md  |  2 +-
 gcc/config/aarch64/thunderx3t110.md |  4 +-
 gcc/config/aarch64/tsv110.md|  2 +-
 gcc/config/arm/arm-fixed.md |  2 +-
 gcc/config/arm/arm.c|  3 +-
 gcc/config/arm/arm.md   | 39 ++--
 gcc/config/arm/arm1020e.md  |  2 +-
 gcc/config/arm/arm1026ejs.md|  2 +-
 gcc/config/arm/arm1136jfs.md|  2 +-
 gcc/config/arm/arm926ejs.md |  2 +-
 gcc/config/arm/common.md| 37 +++
 gcc/config/arm/cortex-a15.md|  2 +-
 gcc/config/arm/cortex-a17.md|  2 +-
 gcc/config/arm/cortex-a5.md |  2 +-
 gcc/config/arm/cortex-a53.md|  2 +-
 gcc/config/arm/cortex-a57.md|  2 +-
 gcc/config/arm/cortex-a7.md |  2 +-
 gcc/config/arm/cortex-a8.md |  2 +-
 gcc/config/arm/cortex-a9.md |  2 +-
 gcc/config/arm/cortex-m4.md |  2 +-