[PATCH v3] AArch64: Improve GOT addressing

Richard Sandiford richard.sandiford@arm.com
Tue Nov 2 18:55:12 GMT 2021


Wilco Dijkstra <Wilco.Dijkstra@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."


More information about the Gcc-patches mailing list