From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate.crashing.org (gate.crashing.org [63.228.1.57]) by sourceware.org (Postfix) with ESMTP id 8155E3858D35 for ; Thu, 6 Aug 2020 22:34:04 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 8155E3858D35 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=kernel.crashing.org Authentication-Results: sourceware.org; spf=fail smtp.mailfrom=segher@kernel.crashing.org Received: from gate.crashing.org (localhost.localdomain [127.0.0.1]) by gate.crashing.org (8.14.1/8.14.1) with ESMTP id 076MY4oT030227; Thu, 6 Aug 2020 17:34:04 -0500 Received: (from segher@localhost) by gate.crashing.org (8.14.1/8.14.1/Submit) id 076MY3VG030226; Thu, 6 Aug 2020 17:34:03 -0500 X-Authentication-Warning: gate.crashing.org: segher set sender to segher@kernel.crashing.org using -f Date: Thu, 6 Aug 2020 17:34:03 -0500 From: Segher Boessenkool To: Alan Modra Cc: gcc-patches@gcc.gnu.org Subject: Re: [RS6000] PR96493, powerpc local call linkage failure Message-ID: <20200806223403.GX6753@gate.crashing.org> References: <20200806132818.GF15695@bubble.grove.modra.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20200806132818.GF15695@bubble.grove.modra.org> User-Agent: Mutt/1.4.2.3i X-Spam-Status: No, score=-8.3 required=5.0 tests=BAYES_00, JMQ_SPF_NEUTRAL, KAM_DMARC_STATUS, KAM_STOCKGEN, RCVD_IN_DNSWL_NONE, TXREP, T_SPF_HELO_PERMERROR, T_SPF_PERMERROR autolearn=no autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) 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: Thu, 06 Aug 2020 22:34:05 -0000 Hi! On Thu, Aug 06, 2020 at 10:58:18PM +0930, Alan Modra wrote: > This corrects current_file_function_operand, an operand predicate used > to determine whether a symbol_ref is safe to use with the local_call > patterns. Calls between pcrel and non-pcrel code need to go via > linker stubs. In the case of non-pcrel code to pcrel the stub saves > r2 but there needs to be a nop after the branch for the r2 restore. > So the local_call patterns can't be used there. Okay. > For pcrel code to > non-pcrel the local_call patterns could still be used, but I thought > it better to not use them since the call isn't direct. Code generated > by the corresponding call_nonlocal_aix for pcrel is identical anyway. Hrm. > Should I rename current_file_function_operand to something more > meaningful before committing? direct_local_call_operand perhaps? As a separate patch, either before or after this one. And maybe a better name than that as well, direct_local_call_operand isn't great? In the same vein, maybe the local_call pattern names should be changed? Because this isn't used just for local calls anymore; instead, the defining characteristic is whether there is a restore of r2 after the call (whether there might be any such restore needed). The pattern names and the operand name ideally would be obviously related? > --- a/gcc/config/rs6000/predicates.md > +++ b/gcc/config/rs6000/predicates.md > @@ -1051,7 +1051,12 @@ > && !((DEFAULT_ABI == ABI_AIX > || DEFAULT_ABI == ABI_ELFv2) > && (SYMBOL_REF_EXTERNAL_P (op) > - || SYMBOL_REF_WEAK (op)))"))) > + || SYMBOL_REF_WEAK (op))) > + && !(DEFAULT_ABI == ABI_ELFv2 > + && SYMBOL_REF_DECL (op) != NULL > + && TREE_CODE (SYMBOL_REF_DECL (op)) == FUNCTION_DECL > + && (rs6000_fndecl_pcrel_p (SYMBOL_REF_DECL (op)) > + != rs6000_pcrel_p (cfun)))"))) This condition is much too complex like that... can you factor it out to a code block, perhaps? Or maybe there should be an actual helper function. > --- /dev/null > +++ b/gcc/testsuite/gcc.target/powerpc/pr96493.c > @@ -0,0 +1,28 @@ > +/* { dg-do run } */ > +/* { dg-options "-mdejagnu-cpu=powerpc64 -O2" } */ That is not a -mcpu= value you should ever use. Please just pick a real existing CPU, maybe p7 or p8 since this requires ELFv2 anyway? Or, what does it need here? It isn't clear to me. But you don't want a pseudo- POWER3 with ELFv2 :-) The patch is okay for trunk (and backports later) if you fix the testcase (the renames and other improvements can be done later, and do not need backporting). Thanks! Segher