From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 127976 invoked by alias); 11 Aug 2016 07:07:19 -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 127961 invoked by uid 89); 11 Aug 2016 07:07:18 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=0.3 required=5.0 tests=BAYES_50,RP_MATCHES_RCVD,SPF_PASS autolearn=ham version=3.3.2 spammy=sk:referen, regtest, subtract, component's X-HELO: mx2.suse.de Received: from mx2.suse.de (HELO mx2.suse.de) (195.135.220.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (CAMELLIA256-SHA encrypted) ESMTPS; Thu, 11 Aug 2016 07:07:17 +0000 Received: from relay2.suse.de (charybdis-ext.suse.de [195.135.220.254]) by mx2.suse.de (Postfix) with ESMTP id C5852AB07; Thu, 11 Aug 2016 07:07:13 +0000 (UTC) Date: Thu, 11 Aug 2016 07:07:00 -0000 From: Richard Biener To: Bernd Edlinger cc: Eric Botcazou , "gcc-patches@gcc.gnu.org" , Jeff Law , Jakub Jelinek Subject: Re: [PATCH] Fix unaligned access when predictive commoning (PR 71083) In-Reply-To: Message-ID: References: <7783559.v48Yu3xPbD@arcturus.home> User-Agent: Alpine 2.11 (LSU 23 2013-08-11) MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII X-SW-Source: 2016-08/txt/msg00880.txt.bz2 On Wed, 10 Aug 2016, Bernd Edlinger wrote: > On 08/10/16 14:29, Richard Biener wrote: > > On Tue, 9 Aug 2016, Bernd Edlinger wrote: > >> We know that the difference between the innermost ref > >> and the outer ref is byte-aligned, but we do not know > >> that the same is true for offset between the COMPONENT_REF > >> and the outer ref. > >> > >> I mean boff is essentially the difference between > >> bitpos of get_inner_reference(exp) and > >> bitpos of get_inner_reference(TREE_OPERAND (exp, 0)) > >> > >> This would be exposed by my patch, because previously > >> we always generated BIT_FIELD_REFS, with bit-offset 0, > >> but the MEM_REF at the byte-offset there is in all likelihood > >> pretty much unaligned, the MEM_REF at the COMPONENT_REF > >> is likely more aligned and allows better code for ARM processors, > >> but only if the MEM_REF is at a byte-aligned offset at all, > >> otherwise the whole transformation would be wrong. > > > > Note that the important thing to ensure is that the access the > > MEM_REF performs is correct TBAA-wise which means you either > > have to use alias-set zero (ptr_type_node offset) or _not_ > > shuffle the offset arbitrarily between the MEM_REF and the > > components you wrap it in. > > > > Richard. > > > > Yes, the patch exactly replicates the outermost COMPONENT_REF and > subtracts the component's byte-offset from the MEM_REF's address, > and the MEM_REF uses the pointer type of the inner reference. > > In the case without bitfields and the Ada bitfields the patch changes > nothing, except we build an aligned type out of TREE_TYPE (DR_REF (dr)) > and get_object_alignment (DR_REF (dr)). > > In the case with a component_ref that is byte-aligned > we subtract the component byte offset from the address > before the MEM_REF is constructed. And the > alias_ptr is of type reference_alias_ptr_type > (TREE_OPERAND (DR_REF (dr), 0)) and again the alignment > from get_object_alignment (TREE_OPERAND (DR_REF (dr), 0) > so that should be exactly type-correct from TBAA's perspective. > > > Attached a new version of the patch with an improved comment, > and the new Ada test cases. > > > Bootstrap and reg-test on x86_64-pc-linux-gnu without regression. > Is it OK for trunk? The patch looks mostly ok, but + else + { + boff >>= LOG2_BITS_PER_UNIT; + boff += tree_to_uhwi (component_ref_field_offset (ref)); + coff = size_binop (MINUS_EXPR, coff, ssize_int (boff)); how can we be sure that component_ref_field_offset is an INTEGER_CST? At least Ada can have variably-length fields before a bitfield. We'd need to apply component_ref_field_offset to off in that case. Which makes me wonder if we can simply avoid the COMPONENT_REF path in a first iteration of the patch and always build a BIT_FIELD_REF? It should solve the correctness issues as well and be more applicable for branches. Thanks, Richard.