Wilco Dijkstra <wilco.dijks...@arm.com> writes:
> Hi Richard,
>
>> - Why do we rewrite the constant moves after reload into ldr_got_small_sidi
>> and ldr_got_small_<mode>? Couldn't we just get the move patterns to
>> output the sequence directly?
>
> That's possible too, however it makes the movsi/di patterns more complex.
Yeah, it certainly does that, but it also makes the other code
significantly simpler. :-)
> See version v4 below.
Thanks, this looks good apart from a couple of nits:
> […]
> diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md
> index
> 7085cd4a51dc4c22def9b95f2221b3847603a9e5..9d38269e9429597b825838faf9f241216cc6ab47
> 100644
> --- a/gcc/config/aarch64/aarch64.md
> +++ b/gcc/config/aarch64/aarch64.md
> @@ -1263,8 +1263,8 @@ (define_expand "mov<mode>"
> )
>
> (define_insn_and_split "*movsi_aarch64"
> - [(set (match_operand:SI 0 "nonimmediate_operand" "=r,k,r,r,r,r, r,w, m, m,
> r, r, w,r,w, w")
> - (match_operand:SI 1 "aarch64_mov_operand" "
> r,r,k,M,n,Usv,m,m,rZ,w,Usa,Ush,rZ,w,w,Ds"))]
> + [(set (match_operand:SI 0 "nonimmediate_operand" "=r,k,r,r,r,r, r,w, m, m,
> r, r, r, w,r,w, w")
> + (match_operand:SI 1 "aarch64_mov_operand" "
> r,r,k,M,n,Usv,m,m,rZ,w,Usw,Usa,Ush,rZ,w,w,Ds"))]
> "(register_operand (operands[0], SImode)
> || aarch64_reg_or_zero (operands[1], SImode))"
> "@
> @@ -1278,6 +1278,7 @@ (define_insn_and_split "*movsi_aarch64"
> ldr\\t%s0, %1
> str\\t%w1, %0
> str\\t%s1, %0
> + * return \"adrp\\t%x0, %A1\;ldr\\t%w0, [%x0, %L1]\";
The * return stuff shouldn't be necessary here. E.g. the SVE MOVPRFX
alternatives directly use \; in @ alternatives.
> adr\\t%x0, %c1
> adrp\\t%x0, %A1
> fmov\\t%s0, %w1
> @@ -1293,13 +1294,15 @@ (define_insn_and_split "*movsi_aarch64"
> }"
> ;; The "mov_imm" type for CNT is just a placeholder.
> [(set_attr "type" "mov_reg,mov_reg,mov_reg,mov_imm,mov_imm,mov_imm,load_4,
> -
> load_4,store_4,store_4,adr,adr,f_mcr,f_mrc,fmov,neon_move")
> - (set_attr "arch" "*,*,*,*,*,sve,*,fp,*,fp,*,*,fp,fp,fp,simd")]
> +
> load_4,store_4,store_4,load_4,adr,adr,f_mcr,f_mrc,fmov,neon_move")
> + (set_attr "arch" "*,*,*,*,*,sve,*,fp,*,fp,*,*,*,fp,fp,fp,simd")
> + (set_attr "length" "4,4,4,4,*, 4,4, 4,4, 4,8,4,4, 4, 4, 4, 4")
> +]
> )
>
> (define_insn_and_split "*movdi_aarch64"
> - [(set (match_operand:DI 0 "nonimmediate_operand" "=r,k,r,r,r,r,r, r,w,
> m,m, r, r, w,r,w, w")
> - (match_operand:DI 1 "aarch64_mov_operand" "
> r,r,k,N,M,n,Usv,m,m,rZ,w,Usa,Ush,rZ,w,w,Dd"))]
> + [(set (match_operand:DI 0 "nonimmediate_operand" "=r,k,r,r,r,r,r, r,w,
> m,m, r, r, r, w,r,w, w")
> + (match_operand:DI 1 "aarch64_mov_operand" "
> r,r,k,N,M,n,Usv,m,m,rZ,w,Usw,Usa,Ush,rZ,w,w,Dd"))]
> "(register_operand (operands[0], DImode)
> || aarch64_reg_or_zero (operands[1], DImode))"
> "@
> @@ -1314,13 +1317,14 @@ (define_insn_and_split "*movdi_aarch64"
> ldr\\t%d0, %1
> str\\t%x1, %0
> str\\t%d1, %0
> + * return TARGET_ILP32 ? \"adrp\\t%0, %A1\;ldr\\t%w0, [%0, %L1]\" :
> \"adrp\\t%0, %A1\;ldr\\t%0, [%0, %L1]\";
> adr\\t%x0, %c1
> adrp\\t%x0, %A1
> fmov\\t%d0, %x1
> fmov\\t%x0, %d1
> fmov\\t%d0, %d1
> * return aarch64_output_scalar_simd_mov_immediate (operands[1], DImode);"
> - "(CONST_INT_P (operands[1]) && !aarch64_move_imm (INTVAL (operands[1]),
> DImode))
> + "CONST_INT_P (operands[1]) && !aarch64_move_imm (INTVAL (operands[1]),
> DImode)
> && REG_P (operands[0]) && GP_REGNUM_P (REGNO (operands[0]))"
> [(const_int 0)]
> "{
> @@ -1329,9 +1333,10 @@ (define_insn_and_split "*movdi_aarch64"
> }"
> ;; The "mov_imm" type for CNTD is just a placeholder.
> [(set_attr "type" "mov_reg,mov_reg,mov_reg,mov_imm,mov_imm,mov_imm,mov_imm,
> - load_8,load_8,store_8,store_8,adr,adr,f_mcr,f_mrc,fmov,
> - neon_move")
> - (set_attr "arch" "*,*,*,*,*,*,sve,*,fp,*,fp,*,*,fp,fp,fp,simd")]
> + load_8,load_8,store_8,store_8,load_8,adr,adr,f_mcr,f_mrc,
> + fmov,neon_move")
> + (set_attr "arch" "*,*,*,*,*,*,sve,*,fp,*,fp,*,*,*,fp,fp,fp,simd")
> + (set_attr "length" "4,4,4,4,4,*, 4,4, 4,4, 4,8,4,4, 4, 4, 4, 4")]
> )
>
> (define_insn "insv_imm<mode>"
> @@ -6707,29 +6712,6 @@ (define_insn "add_losym_<mode>"
> [(set_attr "type" "alu_imm")]
> )
>
> -(define_insn "ldr_got_small_<mode>"
> - [(set (match_operand:PTR 0 "register_operand" "=r")
> - (unspec:PTR [(mem:PTR (lo_sum:PTR
> - (match_operand:PTR 1 "register_operand" "r")
> - (match_operand:PTR 2 "aarch64_valid_symref"
> "S")))]
> - UNSPEC_GOTSMALLPIC))]
> - ""
> - "ldr\\t%<w>0, [%1, #:got_lo12:%c2]"
> - [(set_attr "type" "load_<ldst_sz>")]
> -)
> -
> -(define_insn "ldr_got_small_sidi"
> - [(set (match_operand:DI 0 "register_operand" "=r")
> - (zero_extend:DI
> - (unspec:SI [(mem:SI (lo_sum:DI
> - (match_operand:DI 1 "register_operand" "r")
> - (match_operand:DI 2 "aarch64_valid_symref"
> "S")))]
> - UNSPEC_GOTSMALLPIC)))]
> - "TARGET_ILP32"
> - "ldr\\t%w0, [%1, #:got_lo12:%c2]"
> - [(set_attr "type" "load_4")]
> -)
> -
> (define_insn "ldr_got_small_28k_<mode>"
> [(set (match_operand:PTR 0 "register_operand" "=r")
> (unspec:PTR [(mem:PTR (lo_sum:PTR
> diff --git a/gcc/config/aarch64/constraints.md
> b/gcc/config/aarch64/constraints.md
> index
> 3b49b452119c49320020fa9183314d9a25b92491..985fa2217e6a044b1eb2adc886864f63a1f9f9b2
> 100644
> --- a/gcc/config/aarch64/constraints.md
> +++ b/gcc/config/aarch64/constraints.md
> @@ -152,6 +152,14 @@ (define_constraint "Usa"
> (match_test "aarch64_symbolic_address_p (op)")
> (match_test "aarch64_mov_operand_p (op, GET_MODE (op))")))
>
> +(define_constraint "Usw"
> + "@internal
> + A constraint that matches a small GOT access."
> + (and (match_code "symbol_ref")
> + (match_test "aarch64_symbolic_address_p (op)")
This test is redundant, since aarch64_symbolic_address_p is always
true for symbol_refs.
OK with those changes, thanks.
Richard
> + (match_test "aarch64_classify_symbolic_expression (op)
> + == SYMBOL_SMALL_GOT_4G")))
> +
> (define_constraint "Uss"
> "@internal
> A constraint that matches an immediate shift constant in SImode."
