From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 24047 invoked by alias); 2 Apr 2012 11:52:37 -0000 Received: (qmail 24038 invoked by uid 22791); 2 Apr 2012 11:52:35 -0000 X-SWARE-Spam-Status: No, hits=-6.0 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,KHOP_THREADED,RCVD_IN_DNSWL_HI,T_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; Mon, 02 Apr 2012 11:52:22 +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 7A1428C061; Mon, 2 Apr 2012 13:52:20 +0200 (CEST) Date: Mon, 02 Apr 2012 11:52:00 -0000 From: Richard Guenther To: Martin Jambor Cc: GCC Patches , ebotcazou@adacore.com Subject: Re: [PATCH] Dissociate store_expr's temp from exp so that it is not marked as addressable In-Reply-To: <20120331070601.GA24942@virgil.arch.suse.de> Message-ID: References: <20120329232205.GB2817@virgil.arch.suse.de> <20120331070601.GA24942@virgil.arch.suse.de> 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: 2012-04/txt/msg00034.txt.bz2 On Sat, 31 Mar 2012, Martin Jambor wrote: > Hi, > > On Fri, Mar 30, 2012 at 10:03:59AM +0200, Richard Guenther wrote: > > On Fri, 30 Mar 2012, Martin Jambor wrote: > > > > > Hi, > > > > > > when testing a patch of mine on sparc64-linux, I came across an Ada > > > bootstrap failure due to a structure DECL which was marked addressable > > > but had a register DECL_RTL (and therefore mem_ref_refers_to_non_mem_p > > > failed to trigger on it). > > > > > > Mode of the structure was TI (16 bytes int) and it was mistakenly > > > marked as addressable during expansion of an assignment statement in > > > which a 12 byte portion of it was copied to another structure with > > > BLKmode. Specifically, this happened because expand_assignment called > > > store_expr which loaded the required portion to temporary 12 byte > > > BLKmode MEM_P variable and then called emit_block_move from the > > > temporary to the destination. emit_block_move_hints then marked > > > MEM_EXPR of the temp as addressable because it handled the copy by > > > emitting a library call. And MEM_EXPR pointed to the DECL of the > > > source of the assignment which I believe is the bug, thus this patch > > > re-sets MEM_EXPR of temp in these cases. > > > > > > However, if anybody believes the main issue is elsewhere and another > > > component of this chain of events needs to be fixed, I'll be happy to > > > come up with another patch. so far this patch has passed bootstrap > > > and testing on x86_64-linux and helped my patch which uncovered this > > > issue to reach stage 3 of bootstrap. > > > > > > What do you think, is it OK for trunk? > > > > Hmm, I think this should be done at the point we create the mem - where > > does that happen? > > The function gets it by simply calling expand_expr_real: > > temp = expand_expr_real (exp, tmp_target, GET_MODE (target), > (call_param_p > ? EXPAND_STACK_PARM : EXPAND_NORMAL), > &alt_rtl); > > I have reproduced the issue again and looked at how expand_expr_real_1 > comes up with the MEM attributes in the COMPONENT_REF case. > The memory-backed temporary is created in: > > /* Otherwise, if this is a constant or the object is not in memory > and need be, put it there. */ > else if (CONSTANT_P (op0) || (!MEM_P (op0) && must_force_mem)) > { > tree nt = build_qualified_type (TREE_TYPE (tem), > (TYPE_QUALS (TREE_TYPE (tem)) > | TYPE_QUAL_CONST)); > memloc = assign_temp (nt, 1, 1, 1); > emit_move_insn (memloc, op0); > op0 = memloc; > } > > and at the end of the case, the bitpos is added by adjust_address, > followed by set_mem_attributes (op0, exp, 0); > > Indeed it seems that we should not be calling set_mem_attributes if > op0 is based on such temporary... or perhaps just make sure that we > only clear MEM_EXPR afterwards? Yes, either way I suppose. The following also looks dangerous to me: /* If OFFSET is making OP0 more aligned than BIGGEST_ALIGNMENT, record its alignment as BIGGEST_ALIGNMENT. */ if (MEM_P (op0) && bitpos == 0 && offset != 0 && is_aligning_offset (offset, tem)) set_mem_align (op0, BIGGEST_ALIGNMENT); Maybe we can fall through most of the rest of the function if we canonicalized in the above way? Eric? Thanks, Richard.