From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by sourceware.org (Postfix) with ESMTP id 198CA3858402 for ; Tue, 2 Nov 2021 18:55:15 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 198CA3858402 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id B2B1A11B3; Tue, 2 Nov 2021 11:55:14 -0700 (PDT) Received: from localhost (e121540-lin.manchester.arm.com [10.32.98.88]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 1FDC83F7B4; Tue, 2 Nov 2021 11:55:14 -0700 (PDT) From: Richard Sandiford To: Wilco Dijkstra Mail-Followup-To: Wilco Dijkstra , Kyrylo Tkachov , GCC Patches , richard.sandiford@arm.com Cc: Kyrylo Tkachov , GCC Patches Subject: Re: [PATCH v3] AArch64: Improve GOT addressing References: Date: Tue, 02 Nov 2021 18:55:12 +0000 In-Reply-To: (Wilco Dijkstra's message of "Tue, 2 Nov 2021 18:41:43 +0000") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-11.6 required=5.0 tests=BAYES_00, GIT_PATCH_0, KAM_DMARC_STATUS, KAM_STOCKGEN, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 02 Nov 2021 18:55:16 -0000 Wilco Dijkstra writes: > Hi Richard, > >> - Why do we rewrite the constant moves after reload into ldr_got_small_s= idi >> and ldr_got_small_? 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: > [=E2=80=A6] > diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md > index 7085cd4a51dc4c22def9b95f2221b3847603a9e5..9d38269e9429597b825838faf= 9f241216cc6ab47 100644 > --- a/gcc/config/aarch64/aarch64.md > +++ b/gcc/config/aarch64/aarch64.md > @@ -1263,8 +1263,8 @@ (define_expand "mov" > ) > > (define_insn_and_split "*movsi_aarch64" > - [(set (match_operand:SI 0 "nonimmediate_operand" "=3Dr,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" "=3Dr,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,loa= d_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,fmo= v,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" "=3Dr,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" "=3Dr,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], DImod= e);" > - "(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,fm= ov, > - 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" > @@ -6707,29 +6712,6 @@ (define_insn "add_losym_" > [(set_attr "type" "alu_imm")] > ) > > -(define_insn "ldr_got_small_" > - [(set (match_operand:PTR 0 "register_operand" "=3Dr") > - (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%0, [%1, #:got_lo12:%c2]" > - [(set_attr "type" "load_")] > -) > - > -(define_insn "ldr_got_small_sidi" > - [(set (match_operand:DI 0 "register_operand" "=3Dr") > - (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_" > [(set (match_operand:PTR 0 "register_operand" "=3Dr") > (unspec:PTR [(mem:PTR (lo_sum:PTR > diff --git a/gcc/config/aarch64/constraints.md b/gcc/config/aarch64/const= raints.md > index 3b49b452119c49320020fa9183314d9a25b92491..985fa2217e6a044b1eb2adc88= 6864f63a1f9f9b2 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) > + =3D=3D SYMBOL_SMALL_GOT_4G"))) > + > (define_constraint "Uss" > "@internal > A constraint that matches an immediate shift constant in SImode."