From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 101176 invoked by alias); 21 Mar 2019 11:16:14 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 101025 invoked by uid 89); 21 Mar 2019 11:16:04 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-11.2 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_2,GIT_PATCH_3,SPF_PASS autolearn=ham version=3.3.1 spammy=sk:STRICT_, enhanced, armv5, MEM_P X-HELO: mx1.suse.de Received: from mx2.suse.de (HELO mx1.suse.de) (195.135.220.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 21 Mar 2019 11:16:02 +0000 Received: from relay2.suse.de (unknown [195.135.220.254]) by mx1.suse.de (Postfix) with ESMTP id 29CE0AB48; Thu, 21 Mar 2019 11:15:55 +0000 (UTC) Date: Thu, 21 Mar 2019 11:26:00 -0000 From: Richard Biener To: Bernd Edlinger cc: "gcc-patches@gcc.gnu.org" , Richard Earnshaw , Ramana Radhakrishnan , Kyrill Tkachov , Eric Botcazou Subject: Re: [PATCHv2] Fix not 8-byte aligned ldrd/strd on ARMv5 (PR 89544) In-Reply-To: Message-ID: References: User-Agent: Alpine 2.20 (LSU 67 2015-01-07) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII X-SW-Source: 2019-03/txt/msg01045.txt.bz2 On Sun, 10 Mar 2019, Bernd Edlinger wrote: > Hi, > > This patch is an update to the previous patch, which takes into account that > the middle-end is not supposed to use the unaligned DI value directly which > was passed in an unaligned stack slot due to the AAPCS parameter passing rules. > > The patch works by changing use_register_for_decl to return false if the > incoming RTL is not sufficiently aligned on a STRICT_ALIGNMENT target, > as if the address of the parameter was taken (which is TREE_ADDRESSABLE). > So not taking the address of the parameter is a necessary condition > for the wrong-code in PR 89544. > > It works together with this check in assign_parm_adjust_stack_rtl: > /* If we can't trust the parm stack slot to be aligned enough for its > ultimate type, don't use that slot after entry. We'll make another > stack slot, if we need one. */ > if (stack_parm > && ((STRICT_ALIGNMENT > && GET_MODE_ALIGNMENT (data->nominal_mode) > MEM_ALIGN (stack_parm)) > ... > stack_param = NULL > > This makes assign_parms use assign_parm_setup_stack instead of > assign_parm_setup_reg, and the latter does assign a suitably aligned > stack slot, because stack_param == NULL by now, and uses emit_block_move > which avoids the issue with the back-end. > > Additionally, to prevent unnecessary performance regressions, > assign_parm_find_stack_rtl is enhanced to make use of a possible larger > alignment if the parameter was passed in an aligned stack slot without > the ABI requiring it. > > > Bootstrapped and reg-tested with all languages on arm-linux-gnueabihf. > Is it OK for trunk? I think the assign_parm_find_stack_rtl is not appropriate at this stage, I am also missing an update to the comment of the block you change. It also changes code I'm not familar enough with to review... Finally... Index: gcc/function.c =================================================================== --- gcc/function.c (revision 269264) +++ gcc/function.c (working copy) @@ -2210,6 +2210,12 @@ use_register_for_decl (const_tree decl) if (DECL_MODE (decl) == BLKmode) return false; + if (STRICT_ALIGNMENT && TREE_CODE (decl) == PARM_DECL + && DECL_INCOMING_RTL (decl) && MEM_P (DECL_INCOMING_RTL (decl)) + && GET_MODE_ALIGNMENT (DECL_MODE (decl)) + > MEM_ALIGN (DECL_INCOMING_RTL (decl))) + return false; + /* If -ffloat-store specified, don't put explicit float variables into registers. */ /* ??? This should be checked after DECL_ARTIFICIAL, but tree-ssa I wonder if it is necessary to look at DECL_INCOMING_RTL here and why such RTL may not exist? That is, iff DECL_INCOMING_RTL doesn't exist then shouldn't we return false for safety reasons? Similarly the very same issue should exist on x86_64 which is !STRICT_ALIGNMENT, it's just the ABI seems to provide the appropriate alignment on the caller side. So the STRICT_ALIGNMENT check is a wrong one. Which makes me think that a proper fix is not here, but in target(hook) code. Changing use_register_for_decl sounds odd anyways since if we return true we for the testcase still end up in memory, no? The hunk obviously misses a comment since the effect that this will cause a copy to be emitted isn't obvious (and relying on this probably fragile). Richard.