On Mon, Mar 06, 2017 at 05:08:57AM +0000, Hurugalawadi, Naveen wrote:
> Hi,
>
> Please find attached the patch that adds "addr_type" attribute
> for AArch64.
>
> The patch doesn't change spec but improve other benchmarks.
What am I missing - you add a new function which is never called? This patch
does nothing!
Should this have been in series with a scheduling model change?
As it is, this is not OK - it just adds dead code.
When this was first proposed as an approach, Andrew replied:
> Since this is only about post and pre increment being split into two
> micro-ops. I suspect something like what rs600 back-end does is
> better:
> ;; Does this instruction use update addressing?
> ;; This is used for load and store insns. See the comments for "indexed".
> (define_attr "update" "no,yes"
> (if_then_else (ior (match_operand 0 "update_address_mem")
> (match_operand 1 "update_address_mem"))
> (const_string "yes")
> (const_string "no")))
>
> Where update_address_mem is defined as:
> ;; Return 1 if the operand is a MEM with an update-form address. This may
> ;; also include update-indexed form.
> (define_special_predicate "update_address_mem"
> (match_test "(MEM_P (op)
> && (GET_CODE (XEXP (op, 0)) == PRE_INC
> || GET_CODE (XEXP (op, 0)) == PRE_DEC
> || GET_CODE (XEXP (op, 0)) == PRE_MODIFY))"))
>
>
> Note you need to include the POST ones for AARCH64 but it should be
> similar enough.
> And then you just check the attr update.
Which still sounds like the better approach!
Thanks,
James
>
> Bootstrapped and Regression tested on aarch64-thunder-linux.
> Please review the patch and let us know if its okay for Stage-1?
>
> Thanks,
> Naveen
>
> 2017-03-06 Julian Brown <jul...@codesourcery.com>
> Naveen H.S <naveen.hurugalaw...@cavium.com>
>
> * config/aarch64/aarch64-protos.h (AARCH64_ADDR_REG_IMM)
> (AARCH64_ADDR_REG_WB, AARCH64_ADDR_REG_REG, AARCH64_ADDR_REG_SHIFT)
> (AARCH64_ADDR_REG_EXT, AARCH64_ADDR_REG_SHIFT_EXT, AARCH64_ADDR_LO_SUM)
> (AARCH64_ADDR_SYMBOLIC): New.
> (aarch64_mem_type_p): Add prototype.
> * config/aarch64/aarch64.c (aarch64_mem_type_p): New function.
> * config/aarch64/aarch64.md (addr_type): New attribute.
> (prefetch, *mov<mode>_aarch64, *movsi_aarch64, *movdi_aarch64)
> (*movti_aarch64, *movtf_aarch64, *movsf_aarch64, *movdf_aarch64)
> (load_pairsi, load_pairdi, store_pairsi, store_pairdi, load_pairsf)
> (load_pairdf, store_pairsf)
> (store_pairdf, loadwb_pair<GPI:mode>_<P:mode>)
> (storewb_pair<GPI:mode>_<P:mode>, extendsidi2_aarch64)
> (*load_pair_extendsidi2_aarch64, *zero_extendsidi2_aarch64)
> (*load_pair_zero_extendsidi2_aarch64)
> (*extend<SHORT:mode><GPI:mode>2_aarch64)
> (*zero_extend<SHORT:mode><GPI:mode>2_aarch64)
> (ldr_got_small_<mode>, ldr_got_small_sidi, ldr_got_tiny)
> (tlsie_small_<mode>, tlsie_small_sidi): Add addr_type attribute.
> diff --git a/gcc/config/aarch64/aarch64-protos.h
> b/gcc/config/aarch64/aarch64-protos.h
> index 9543f8c..e045df8 100644
> --- a/gcc/config/aarch64/aarch64-protos.h
> +++ b/gcc/config/aarch64/aarch64-protos.h
> @@ -299,6 +299,19 @@ enum aarch64_parse_opt_result
>
> extern struct tune_params aarch64_tune_params;
>
> +/* Mask bits to use for for aarch64_mem_type_p. Unshifted/shifted index
> + register variants are separated for scheduling purposes because the
> + distinction matters on some cores. */
> +
> +#define AARCH64_ADDR_REG_IMM 0x01
> +#define AARCH64_ADDR_REG_WB 0x02
> +#define AARCH64_ADDR_REG_REG 0x04
> +#define AARCH64_ADDR_REG_SHIFT 0x08
> +#define AARCH64_ADDR_REG_EXT 0x10
> +#define AARCH64_ADDR_REG_SHIFT_EXT 0x20
> +#define AARCH64_ADDR_LO_SUM 0x40
> +#define AARCH64_ADDR_SYMBOLIC 0x80
> +
> HOST_WIDE_INT aarch64_initial_elimination_offset (unsigned, unsigned);
> int aarch64_get_condition_code (rtx);
> bool aarch64_bitmask_imm (HOST_WIDE_INT val, machine_mode);
> @@ -347,6 +360,7 @@ bool aarch64_simd_shift_imm_p (rtx, machine_mode, bool);
> bool aarch64_simd_valid_immediate (rtx, machine_mode, bool,
> struct simd_immediate_info *);
> bool aarch64_split_dimode_const_store (rtx, rtx);
> +bool aarch64_mem_type_p (rtx_insn *, unsigned HOST_WIDE_INT);
> bool aarch64_symbolic_address_p (rtx);
> bool aarch64_uimm12_shift (HOST_WIDE_INT);
> bool aarch64_use_return_insn_p (void);
> diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
> index 714bb79..fa25d43 100644
> --- a/gcc/config/aarch64/aarch64.c
> +++ b/gcc/config/aarch64/aarch64.c
> @@ -4551,6 +4551,88 @@ aarch64_classify_address (struct aarch64_address_info
> *info,
> }
> }
>
> +/* Return TRUE if INSN uses an address that satisfies any of the (non-strict)
> + addressing modes specified by MASK. This is intended for use in
> scheduling
> + models that are sensitive to the form of address used by some particular
> + instruction. */
> +
> +bool
> +aarch64_mem_type_p (rtx_insn *insn, unsigned HOST_WIDE_INT mask)
> +{
> + aarch64_address_info info;
> + bool valid;
> + attr_addr_type addr_type;
> + rtx mem, addr;
> + machine_mode mode;
> +
> + addr_type = get_attr_addr_type (insn);
> +
> + switch (addr_type)
> + {
> + case ADDR_TYPE_WB:
> + info.type = ADDRESS_REG_WB;
> + break;
> +
> + case ADDR_TYPE_LO_SUM:
> + info.type = ADDRESS_LO_SUM;
> + break;
> +
> + case ADDR_TYPE_OP0:
> + case ADDR_TYPE_OP1:
> + extract_insn_cached (insn);
> +
> + mem = recog_data.operand[(addr_type == ADDR_TYPE_OP0) ? 0 : 1];
> +
> + gcc_assert (MEM_P (mem));
> +
> + addr = XEXP (mem, 0);
> + mode = GET_MODE (mem);
> +
> + classify:
> + valid = aarch64_classify_address (&info, addr, mode, MEM, false);
> + if (!valid)
> + return false;
> +
> + break;
> +
> + case ADDR_TYPE_OP0ADDR:
> + case ADDR_TYPE_OP1ADDR:
> + extract_insn_cached (insn);
> +
> + addr = recog_data.operand[(addr_type == ADDR_TYPE_OP0ADDR) ? 0 : 1];
> + mode = DImode;
> + goto classify;
> +
> + case ADDR_TYPE_NONE:
> + return false;
> + }
> +
> + switch (info.type)
> + {
> + case ADDRESS_REG_IMM:
> + return (mask & AARCH64_ADDR_REG_IMM) != 0;
> + case ADDRESS_REG_WB:
> + return (mask & AARCH64_ADDR_REG_WB) != 0;
> + case ADDRESS_REG_REG:
> + if (info.shift == 0)
> + return (mask & AARCH64_ADDR_REG_REG) != 0;
> + else
> + return (mask & AARCH64_ADDR_REG_SHIFT) != 0;
> + case ADDRESS_REG_UXTW:
> + case ADDRESS_REG_SXTW:
> + if (info.shift == 0)
> + return (mask & AARCH64_ADDR_REG_EXT) != 0;
> + else
> + return (mask & AARCH64_ADDR_REG_SHIFT_EXT) != 0;
> + case ADDRESS_LO_SUM:
> + return (mask & AARCH64_ADDR_LO_SUM) != 0;
> + case ADDRESS_SYMBOLIC:
> + return (mask & AARCH64_ADDR_SYMBOLIC) != 0;
> + default:
> + return false;
> + }
> +}
> +
> bool
> aarch64_symbolic_address_p (rtx x)
> {
> diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md
> index 5adc5ed..f3dc65c 100644
> --- a/gcc/config/aarch64/aarch64.md
> +++ b/gcc/config/aarch64/aarch64.md
> @@ -201,6 +201,9 @@
> (const_string "no")
> ] (const_string "yes")))
>
> +(define_attr "addr_type" "none,op0,op1,op0addr,op1addr,lo_sum,wb"
> + (const_string "none"))
> +
> ;; Attribute that specifies whether we are dealing with a branch to a
> ;; label that is far away, i.e. further away than the maximum/minimum
> ;; representable in a signed 21-bits number.
> @@ -542,7 +545,8 @@
>
> return pftype[INTVAL(operands[1])][locality];
> }
> - [(set_attr "type" "load1")]
> + [(set_attr "type" "load1")
> + (set_attr "addr_type" "op0addr")]
> )
>
> (define_insn "trap"
> @@ -998,7 +1002,8 @@
> }
> [(set_attr "type" "mov_reg,mov_imm,neon_move,load1,load1,store1,store1,\
> neon_to_gp<q>,neon_from_gp<q>,neon_dup")
> - (set_attr "simd" "*,*,yes,*,*,*,*,yes,yes,yes")]
> + (set_attr "simd" "*,*,yes,*,*,*,*,yes,yes,yes")
> + (set_attr "addr_type" "*,*,*,op1,op1,op0,op0,*,*,*")]
> )
>
> (define_expand "mov<mode>"
> @@ -1054,7 +1059,8 @@
> }"
> [(set_attr "type"
> "mov_reg,mov_reg,mov_reg,mov_imm,mov_imm,load1,load1,store1,store1,\
> adr,adr,f_mcr,f_mrc,fmov")
> - (set_attr "fp" "*,*,*,*,*,*,yes,*,yes,*,*,yes,yes,yes")]
> + (set_attr "fp" "*,*,*,*,*,*,yes,*,yes,*,*,yes,yes,yes")
> + (set_attr "addr_type" "*,*,*,*,*,op1,op1,op0,op0,*,*,*,*,*")]
> )
>
> (define_insn_and_split "*movdi_aarch64"
> @@ -1088,7 +1094,8 @@
> [(set_attr "type"
> "mov_reg,mov_reg,mov_reg,mov_imm,mov_imm,load1,load1,store1,store1,\
> adr,adr,f_mcr,f_mrc,fmov,neon_move")
> (set_attr "fp" "*,*,*,*,*,*,yes,*,yes,*,*,yes,yes,yes,*")
> - (set_attr "simd" "*,*,*,*,*,*,*,*,*,*,*,*,*,*,yes")]
> + (set_attr "simd" "*,*,*,*,*,*,*,*,*,*,*,*,*,*,yes")
> + (set_attr "addr_type" "*,*,*,*,*,op1,op1,op0,op0,*,*,*,*,*,*")]
> )
>
> (define_insn "insv_imm<mode>"
> @@ -1133,7 +1140,8 @@
> load2,store2,store2,f_loadd,f_stored")
> (set_attr "length" "8,8,8,4,4,4,4,4,4")
> (set_attr "simd" "*,*,*,yes,*,*,*,*,*")
> - (set_attr "fp" "*,*,*,*,*,*,*,yes,yes")]
> + (set_attr "fp" "*,*,*,*,*,*,*,yes,yes")
> + (set_attr "addr_type" "*,*,*,*,op1,op0,op0,op1,op0")]
> )
>
> ;; Split a TImode register-register or register-immediate move into
> @@ -1205,7 +1213,8 @@
> mov\\t%w0, %w1"
> [(set_attr "type" "neon_move,f_mcr,f_mrc,fmov,fconsts,\
> f_loads,f_stores,load1,store1,mov_reg")
> - (set_attr "simd" "yes,*,*,*,*,*,*,*,*,*")]
> + (set_attr "simd" "yes,*,*,*,*,*,*,*,*,*")
> + (set_attr "addr_type" "*,*,*,*,*,op1,op0,op1,op0,*")]
> )
>
> (define_insn "*movdf_aarch64"
> @@ -1226,7 +1235,8 @@
> mov\\t%x0, %x1"
> [(set_attr "type" "neon_move,f_mcr,f_mrc,fmov,fconstd,\
> f_loadd,f_stored,load1,store1,mov_reg")
> - (set_attr "simd" "yes,*,*,*,*,*,*,*,*,*")]
> + (set_attr "simd" "yes,*,*,*,*,*,*,*,*,*")
> + (set_attr "addr_type" "*,*,*,*,*,op1,op0,op1,op0,*")]
> )
>
> (define_insn "*movtf_aarch64"
> @@ -1251,7 +1261,8 @@
> [(set_attr "type" "logic_reg,multiple,f_mcr,f_mrc,neon_move_q,f_mcr,\
> f_loadd,f_stored,load2,store2,store2")
> (set_attr "length" "4,8,8,8,4,4,4,4,4,4,4")
> - (set_attr "simd" "yes,*,*,*,yes,*,*,*,*,*,*")]
> + (set_attr "simd" "yes,*,*,*,yes,*,*,*,*,*,*")
> + (set_attr "addr_type" "*,*,*,*,*,*,op1,op0,op1,op0,op0")]
> )
>
> (define_split
> @@ -1298,7 +1309,8 @@
> ldp\\t%w0, %w2, %1
> ldp\\t%s0, %s2, %1"
> [(set_attr "type" "load2,neon_load1_2reg")
> - (set_attr "fp" "*,yes")]
> + (set_attr "fp" "*,yes")
> + (set_attr "addr_type" "op1")]
> )
>
> (define_insn "load_pairdi"
> @@ -1314,7 +1326,8 @@
> ldp\\t%x0, %x2, %1
> ldp\\t%d0, %d2, %1"
> [(set_attr "type" "load2,neon_load1_2reg")
> - (set_attr "fp" "*,yes")]
> + (set_attr "fp" "*,yes")
> + (set_attr "addr_type" "op1")]
> )
>
>
> @@ -1333,7 +1346,8 @@
> stp\\t%w1, %w3, %0
> stp\\t%s1, %s3, %0"
> [(set_attr "type" "store2,neon_store1_2reg")
> - (set_attr "fp" "*,yes")]
> + (set_attr "fp" "*,yes")
> + (set_attr "addr_type" "op0")]
> )
>
> (define_insn "store_pairdi"
> @@ -1349,7 +1363,8 @@
> stp\\t%x1, %x3, %0
> stp\\t%d1, %d3, %0"
> [(set_attr "type" "store2,neon_store1_2reg")
> - (set_attr "fp" "*,yes")]
> + (set_attr "fp" "*,yes")
> + (set_attr "addr_type" "op0")]
> )
>
> ;; Operands 1 and 3 are tied together by the final condition; so we allow
> @@ -1367,7 +1382,8 @@
> ldp\\t%s0, %s2, %1
> ldp\\t%w0, %w2, %1"
> [(set_attr "type" "neon_load1_2reg,load2")
> - (set_attr "fp" "yes,*")]
> + (set_attr "fp" "yes,*")
> + (set_attr "addr_type" "op1")]
> )
>
> (define_insn "load_pairdf"
> @@ -1383,7 +1399,8 @@
> ldp\\t%d0, %d2, %1
> ldp\\t%x0, %x2, %1"
> [(set_attr "type" "neon_load1_2reg,load2")
> - (set_attr "fp" "yes,*")]
> + (set_attr "fp" "yes,*")
> + (set_attr "addr_type" "op1")]
> )
>
> ;; Operands 0 and 2 are tied together by the final condition; so we allow
> @@ -1401,7 +1418,8 @@
> stp\\t%s1, %s3, %0
> stp\\t%w1, %w3, %0"
> [(set_attr "type" "neon_store1_2reg,store2")
> - (set_attr "fp" "yes,*")]
> + (set_attr "fp" "yes,*")
> + (set_attr "addr_type" "op0")]
> )
>
> (define_insn "store_pairdf"
> @@ -1417,7 +1435,8 @@
> stp\\t%d1, %d3, %0
> stp\\t%x1, %x3, %0"
> [(set_attr "type" "neon_store1_2reg,store2")
> - (set_attr "fp" "yes,*")]
> + (set_attr "fp" "yes,*")
> + (set_attr "addr_type" "op0")]
> )
>
> ;; Load pair with post-index writeback. This is primarily used in function
> @@ -1434,7 +1453,8 @@
> (match_operand:P 5 "const_int_operand" "n"))))])]
> "INTVAL (operands[5]) == GET_MODE_SIZE (<GPI:MODE>mode)"
> "ldp\\t%<w>2, %<w>3, [%1], %4"
> - [(set_attr "type" "load2")]
> + [(set_attr "type" "load2")
> + (set_attr "addr_type" "wb")]
> )
>
> (define_insn "loadwb_pair<GPF:mode>_<P:mode>"
> @@ -1467,7 +1487,8 @@
> (match_operand:GPI 3 "register_operand" "r"))])]
> "INTVAL (operands[5]) == INTVAL (operands[4]) + GET_MODE_SIZE
> (<GPI:MODE>mode)"
> "stp\\t%<w>2, %<w>3, [%0, %4]!"
> - [(set_attr "type" "store2")]
> + [(set_attr "type" "store2")
> + (set_attr "addr_type" "wb")]
> )
>
> (define_insn "storewb_pair<GPF:mode>_<P:mode>"
> @@ -1503,7 +1524,8 @@
> "@
> sxtw\t%0, %w1
> ldrsw\t%0, %1"
> - [(set_attr "type" "extend,load1")]
> + [(set_attr "type" "extend,load1")
> + (set_attr "addr_type" "*,op1")]
> )
>
> (define_insn "*load_pair_extendsidi2_aarch64"
> @@ -1516,7 +1538,8 @@
> XEXP (operands[1], 0),
> GET_MODE_SIZE (SImode)))"
> "ldpsw\\t%0, %2, %1"
> - [(set_attr "type" "load2")]
> + [(set_attr "type" "load2")
> + (set_attr "addr_type" "op1")]
> )
>
> (define_insn "*zero_extendsidi2_aarch64"
> @@ -1526,7 +1549,8 @@
> "@
> uxtw\t%0, %w1
> ldr\t%w0, %1"
> - [(set_attr "type" "extend,load1")]
> + [(set_attr "type" "extend,load1")
> + (set_attr "addr_type" "*,op1")]
> )
>
> (define_insn "*load_pair_zero_extendsidi2_aarch64"
> @@ -1539,7 +1563,8 @@
> XEXP (operands[1], 0),
> GET_MODE_SIZE (SImode)))"
> "ldp\\t%w0, %w2, %1"
> - [(set_attr "type" "load2")]
> + [(set_attr "type" "load2")
> + (set_attr "addr_type" "op1")]
> )
>
> (define_expand "<ANY_EXTEND:optab><SHORT:mode><GPI:mode>2"
> @@ -1555,7 +1580,8 @@
> "@
> sxt<SHORT:size>\t%<GPI:w>0, %w1
> ldrs<SHORT:size>\t%<GPI:w>0, %1"
> - [(set_attr "type" "extend,load1")]
> + [(set_attr "type" "extend,load1")
> + (set_attr "addr_type" "*,op1")]
> )
>
> (define_insn "*zero_extend<SHORT:mode><GPI:mode>2_aarch64"
> @@ -1566,7 +1592,8 @@
> and\t%<GPI:w>0, %<GPI:w>1, <SHORT:short_mask>
> ldr<SHORT:size>\t%w0, %1
> ldr\t%<SHORT:size>0, %1"
> - [(set_attr "type" "logic_imm,load1,load1")]
> + [(set_attr "type" "logic_imm,load1,load1")
> + (set_attr "addr_type" "*,op1,op1")]
> )
>
> (define_expand "<optab>qihi2"
> @@ -5180,7 +5207,8 @@
> UNSPEC_GOTSMALLPIC))]
> ""
> "ldr\\t%<w>0, [%1, #:got_lo12:%a2]"
> - [(set_attr "type" "load1")]
> + [(set_attr "type" "load1")
> + (set_attr "addr_type" "lo_sum")]
> )
>
> (define_insn "ldr_got_small_sidi"
> @@ -5192,7 +5220,8 @@
> UNSPEC_GOTSMALLPIC)))]
> "TARGET_ILP32"
> "ldr\\t%w0, [%1, #:got_lo12:%a2]"
> - [(set_attr "type" "load1")]
> + [(set_attr "type" "load1")
> + (set_attr "addr_type" "lo_sum")]
> )
>
> (define_insn "ldr_got_small_28k_<mode>"
> @@ -5224,7 +5253,8 @@
> UNSPEC_GOTTINYPIC))]
> ""
> "ldr\\t%0, %L1"
> - [(set_attr "type" "load1")]
> + [(set_attr "type" "load1")
> + (set_attr "addr_type" "op1addr")]
> )
>
> (define_insn "aarch64_load_tp_hard"
> @@ -5266,7 +5296,8 @@
> ""
> "adrp\\t%0, %A1\;ldr\\t%<w>0, [%0, #%L1]"
> [(set_attr "type" "load1")
> - (set_attr "length" "8")]
> + (set_attr "length" "8")
> + (set_attr "addr_type" "op1addr")]
> )
>
> (define_insn "tlsie_small_sidi"
> @@ -5277,7 +5308,8 @@
> ""
> "adrp\\t%0, %A1\;ldr\\t%w0, [%0, #%L1]"
> [(set_attr "type" "load1")
> - (set_attr "length" "8")]
> + (set_attr "length" "8")
> + (set_attr "addr_type" "op1addr")]
> )
>
> (define_insn "tlsie_tiny_<mode>"
