From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 3414 invoked by alias); 10 Jan 2013 09:18:20 -0000 Received: (qmail 3405 invoked by uid 22791); 10 Jan 2013 09:18:19 -0000 X-SWARE-Spam-Status: No, hits=-6.2 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,KHOP_THREADED,RCVD_IN_DNSWL_HI,RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from cantor2.suse.de (HELO mx2.suse.de) (195.135.220.15) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Thu, 10 Jan 2013 09:18:13 +0000 Received: from relay2.suse.de (unknown [195.135.220.254]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by mx2.suse.de (Postfix) with ESMTP id 43234A398D; Thu, 10 Jan 2013 10:18:11 +0100 (CET) Date: Thu, 10 Jan 2013 09:18:00 -0000 From: Richard Biener To: Eric Botcazou Cc: gcc-patches@gcc.gnu.org Subject: Re: [PATCH] Fix PR55882 In-Reply-To: <1676133.ox1dRSafXZ@polaris> Message-ID: References: <1676133.ox1dRSafXZ@polaris> User-Agent: Alpine 2.00 (LNX 1167 2008-08-23) MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII 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 X-SW-Source: 2013-01/txt/msg00519.txt.bz2 On Wed, 9 Jan 2013, Eric Botcazou wrote: > > This fixes PR55882 - set_mem_attributes_minus_bitpos misses to > > account for the to-be applied bitpos when computing MEM_ALIGN. > > It extracts alignment from 't' instead of &t - bitpos. > > > > Bootstrapped and tested on x86_64-unknown-linux-gnu, bootstrap > > and regtest running on mips. > > > > Does this look sensible? > > I'm not sure, bitpos isn't taken into account in the other cases when the > alignment is computed. adjust_address_1 is supposed to properly adjust the > alignment by the time the offset is applied. Yes, but alignment adjustment assumes the alignment is that of the actual MEM, not that of the MEM_EXPR. Thus, MEM_ALIGN is the alignment of MEM_EXPR - MEM_OFFSET. It's true that the other paths setting align_computed have the same issue. I can produce a followup (or preparatory patch) that at least removes the code that is redundant. For example /* If this is a decl, set the attributes of the MEM from it. */ if (DECL_P (t)) { attrs.expr = t; attrs.offset_known_p = true; attrs.offset = 0; apply_bitpos = bitpos; new_size = DECL_SIZE_UNIT (t); attrs.align = DECL_ALIGN (t); align_computed = true; } the alignment computation can be handled by the get_object_alignment path just fine, no need to duplicate it here, likewise for the CONSTANT_CLASS_P case. The ARRAY_REF path is scary enough to me (I'd rather drop it completely ... it's probably designed to make more cases covered by nonoverlapping_component_refs_p?). At least handling of DECL/COMPONENT_REF below the ARRAY_REFs should be commonized - for example the COMPONENT_REF case else if (TREE_CODE (t2) == COMPONENT_REF) { attrs.expr = t2; attrs.offset_known_p = false; if (host_integerp (off_tree, 1)) { attrs.offset_known_p = true; attrs.offset = tree_low_cst (off_tree, 1); apply_bitpos = bitpos; } doesn't adjust 't' nor does it compute alignment. So it clearly cannot be that MEM_ALIGN is supposed to be the alignment of MEM_EXPR. That is - the question boils down to _what_ should MEM_ALIGN be the alignment of? In all the rest of the compiler it surely is the alignment of XEXP (mem, 0) - to make it differ from that during a brief period of expand_assignment sounds fragile at least. I'll try to simplify code paths without changing behavior first. Thanks, Richard.