From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 32501 invoked by alias); 17 Nov 2015 00:07:52 -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 32486 invoked by uid 89); 17 Nov 2015 00:07:51 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-3.1 required=5.0 tests=AWL,BAYES_00,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Tue, 17 Nov 2015 00:07:50 +0000 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) by mx1.redhat.com (Postfix) with ESMTPS id 8C6A3C076632; Tue, 17 Nov 2015 00:07:48 +0000 (UTC) Received: from freie.home (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by int-mx13.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id tAH07klw031710 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=NO); Mon, 16 Nov 2015 19:07:47 -0500 Received: from livre.home (livre.home [172.31.160.2]) by freie.home (8.15.2/8.15.2) with ESMTP id tAH07ZOe009991; Mon, 16 Nov 2015 22:07:36 -0200 From: Alexandre Oliva To: Jeff Law Cc: Alan Lawrence , "gcc-patches\@gcc.gnu.org" , Marcus Shawcroft , James Greenhalgh Subject: Re: [PR64164] drop copyrename, integrate into expand References: <20150810082355.GA31149@arm.com> <55C8BFC3.3030603@redhat.com> <55E72D4C.40705@arm.com> <55FC3171.7040509@arm.com> <56420DC4.3070407@arm.com> <564280E0.7090700@redhat.com> <56458448.7030301@redhat.com> Date: Tue, 17 Nov 2015 00:07:00 -0000 In-Reply-To: <56458448.7030301@redhat.com> (Jeff Law's message of "Thu, 12 Nov 2015 23:33:44 -0700") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-SW-Source: 2015-11/txt/msg02018.txt.bz2 On Nov 13, 2015, Jeff Law wrote: > On 11/11/2015 11:10 AM, Alexandre Oliva wrote: >> On Nov 10, 2015, Jeff Law wrote: >> >>>> * function.c (assign_parm_setup_block): Right-shift >>>> upward-padded big-endian args when bypassing the stack slot. >>> Don't you need to check the value of BLOCK_REG_PADDING at runtime? >>> The padding is essentially allowed to vary. >> >> Well, yeah, it's the result of BLOCK_REG_PADDING that tells whether >> upward-padding occurred and shifting is required. >> >>> If you look at the other places where BLOCK_REG_PADDING is used, it's >>> checked in a #ifdef, then again inside a if conditional. >> >> That's what I do in the patch too. > ? I don't see the runtime check in your patch. I see a couple > gcc_asserts, but no runtime check of BLOCK_REG_PADDING. The check is not in my patch, indeed. That's because the previous block performs the runtime check, and it only lets through two cases: the one we handle, and the one nobody uses. The previous block tests this: if (mode != BLKmode #ifdef BLOCK_REG_PADDING && (size == UNITS_PER_WORD || (BLOCK_REG_PADDING (mode, data->passed_type, 1) != (BYTES_BIG_ENDIAN ? upward : downward))) #endif ) i.e., whether we know the mode of the passed value, and its word-sized, or its padded such that the passed value is in the lowpart of the word. Since this is in a block that runs when size <= UNITS_PER_WORD, this catches (and works for) all cases of default padding (when BLOCK_REG_PADDING is not defined), and for cases in which BLOCK_REG_PADDING is defined so as to behave like the default padding, at least for smaller-than-word modes. So, since this handles little-endian bytes with upward padding and big-endian bytes with downward padding, what remains to be handled is little-endian bytes with downward padding and big-endian bytes with upward padding. I found no evidence that the former is ever used anywhere, or why anyone would ever force shifting for both REG and MEM use, and I don't see how the code would have dealt with this case anyway, so I left it unhandled. The other case, big-endian bytes with upward padding, is precisely the one that my previous patch broke on AArch64: we have the passed values pushed to the upper part of the REG so that it could be stored in memory as a whole word and then accessed in the smaller mode at the same address. After checking that this is the case at hand, we shift the value as if we stored it in memory as a word and loaded it in the value mode. >> That said, the initial conditions in the if/else-if/else chain for the >> no-larger-than-a-word case cover all of the non-BLOCK_REG_PADDING cases >> correctly, so that, if BLOCK_REG_PADDING is not defined, we can just >> skip the !MEM_P block altogether. That's also the reason why we can go >> straight to shifting when we get there. >> >> I tried to document my reasoning in the comments, but maybe it was still >> too obscure? > Certainly seems that way. Is it your assertion that the new code is > what we want regardless of the *value* of REG_BLOCK_PADDING? Sort of. If we get to that point, there's only one reasonable value of BLOCK_REG_PADDING (*), although there's another possible value that we historically haven't handled and that makes very little sense to support. We'd have silently corrupted it before, while now we'd get an assertion failure. I count that as an improvement, though it's unlikely we'd ever hit it: anyone trying to define BLOCK_REG_PADDING so as to pad small args downward on little-endian bytes would AFAICT soon find out it doesn't work. (*) unless mode is BLKmode, which the newly-added code implicitly excludes by testing that we don't have a MEM, but rather a REG. > Essentially meaning the check in the IF is covering both cases? Among all cases for arguments that are word-sized or smaller, the initial IF (not present in the patch) covers all of the "usual" cases. The remaining blocks, including the one I added, cover the remaining handled case, namely, BIG_ENDIAN_BYTES and upward BLOCK_REG_PADDING, or BLKmode BIG_ENDIAN_BYTES and downward BLOCK_REG_PADDING (that needs the opposite padding when storing to big-endian mem, so that the value can be accessed at the address in which the full word is stored). > What am I missing here? I agree that the way the remaining tests are written doesn't make it clear that they're all handling a single case, which makes things confusing. In part, that's because they really aren't; they also deal with BLKmode MEMs with "usual" padding. But that's not a case that the patch affects, because we don't have BLKmode REGs. Any suggestions on how to improve the comments so that they convey enough of this reasoning to make sense, without our having to write a book :-) on the topic? Thanks, -- Alexandre Oliva, freedom fighter http://FSFLA.org/~lxoliva/ You must be the change you wish to see in the world. -- Gandhi Be Free! -- http://FSFLA.org/ FSF Latin America board member Free Software Evangelist|Red Hat Brasil GNU Toolchain Engineer