From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from xry111.site (xry111.site [IPv6:2001:470:683e::1]) by sourceware.org (Postfix) with ESMTPS id B33373858439 for ; Tue, 19 Jul 2022 14:30:04 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org B33373858439 Received: from [IPv6:240e:358:118f:8200:dc73:854d:832e:2] (unknown [IPv6:240e:358:118f:8200:dc73:854d:832e:2]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange ECDHE (P-256) server-signature ECDSA (P-384) server-digest SHA384) (Client did not present a certificate) (Authenticated sender: xry111@xry111.site) by xry111.site (Postfix) with ESMTPSA id 8C57766933; Tue, 19 Jul 2022 10:29:51 -0400 (EDT) Message-ID: <4d470b84a4ddc51236ad22f9069423d360de2c7a.camel@xry111.site> Subject: Re: [PATCH v1 1/2] LoongArch: Modify the method of obtaining symbolic addresses. From: Xi Ruoyao To: Lulu Cheng , gcc-patches@gcc.gnu.org Cc: i@xen0n.name, xuchenghua@loongson.cn Date: Tue, 19 Jul 2022 22:29:35 +0800 In-Reply-To: <20220719130852.2011955-2-chenglulu@loongson.cn> References: <20220719130852.2011955-1-chenglulu@loongson.cn> <20220719130852.2011955-2-chenglulu@loongson.cn> Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable User-Agent: Evolution 3.44.3 MIME-Version: 1.0 X-Spam-Status: No, score=0.3 required=5.0 tests=BAYES_00, BODY_8BITS, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FROM_SUSPICIOUS_NTLD, LIKELY_SPAM_FROM, PDS_OTHER_BAD_TLD, SPF_HELO_PASS, SPF_PASS, TXREP autolearn=no autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) 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, 19 Jul 2022 14:30:06 -0000 The change seems too large. It would be better to split it into multiple commits (for example, just 3 commits for 1,2,3 below). On Tue, 2022-07-19 at 21:08 +0800, Lulu Cheng wrote: > 1. The original LA macro instruction is split into two instructions to > =C2=A0=C2=A0 obtain the address of the symbol if enable '-mexplicit-reloc= s'. It's better to add some test cases (with dg-final scan-assembler) for this. The test case will also show humans the intended behavior after the change. > 3. Modify the method that calls global functions. From 'la.global + jirl' > =C2=A0=C2=A0 to 'bl'. Why? Does it means we'll rely on the assembler to emit the correct sequence for -fno-plt? Then it would be better to use a pseudo mnemonic like "call" instead of "bl" (because it's not a single "bl" instruction). /* snip */ > =C2=A0static bool > =C2=A0loongarch_valid_index_p (struct loongarch_address_info *info, rtx x= , > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0 machine_mode mode, bool strict_p) > @@ -1881,6 +1978,26 @@ loongarch_classify_address (struct loongarch_addre= ss_info *info, rtx x, > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 info->offset =3D XEXP (x, 1); > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 return (loongarch_valid_base_registe= r_p (info->reg, mode, strict_p) > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0 && loongarch_valid_offset_p (info->offset, mode)); > + > +=C2=A0=C2=A0=C2=A0 case LO_SUM: > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 info->type =3D ADDRESS_LO_SUM; > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 info->reg =3D XEXP (x, 0); > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 info->offset =3D XEXP (x, 1); > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 /* We have to trust the creator of the LO= _SUM to do something vaguely > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 sane.=C2=A0 Target-independen= t code that creates a LO_SUM should also > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 create and verify the matchin= g HIGH.=C2=A0 Target-independent code that > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 adds an offset to a LO_SUM mu= st prove that the offset will not > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 induce a carry.=C2=A0 Failure= to do either of these things would be > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 a bug, and we are not require= d to check for it here.=C2=A0 The MIPS Don't copy MIPS code. > +static bool loongarch_split_move_insn_p (rtx dest, rtx src); Nit: "loongarch_split_move_insn_p" shall start a new line.