From patchwork Mon Oct 29 16:36:24 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Michael Matz X-Patchwork-Id: 195070 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) by ozlabs.org (Postfix) with SMTP id DFB772C0080 for ; Tue, 30 Oct 2012 03:36:41 +1100 (EST) Comment: DKIM? See http://www.dkim.org DKIM-Signature: v=1; a=rsa-sha1; c=relaxed/relaxed; d=gcc.gnu.org; s=default; x=1352133402; h=Comment: DomainKey-Signature:Received:Received:Received:Received:Date: From:To:Cc:Subject:In-Reply-To:Message-ID:References:User-Agent: MIME-Version:Content-Type:Mailing-List:Precedence:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:Sender: Delivered-To; bh=zM3VDzNMaTW7N8PputxyA7phbVA=; b=R3zyKpIefkv47U0 Q86IRpel0OUD0BaeZI9IwOcpl7Ol5W2oGqiG6MAmuxJv2W2+cU6Qxu101sywfD9/ ikvwn251ZiRW2CzLqRnodvizjeieAqQtiNL7gkBgNlpVcFVXEciDGukdZaEaPqbw 0X9sBX7F1hZKbIgLqoXMA5JpbqpI= Comment: DomainKeys? See http://antispam.yahoo.com/domainkeys DomainKey-Signature: a=rsa-sha1; q=dns; c=nofws; s=default; d=gcc.gnu.org; h=Received:Received:X-SWARE-Spam-Status:X-Spam-Check-By:Received:Received:Date:From:To:Cc:Subject:In-Reply-To:Message-ID:References:User-Agent:MIME-Version:Content-Type:X-IsSubscribed:Mailing-List:Precedence:List-Id:List-Unsubscribe:List-Archive:List-Post:List-Help:Sender:Delivered-To; b=PtmRhH2TSAwTxFdWMjyJ70lCtk0vBykSGE6+7SnxsTCNwFoqf9wK/9FLRTJ+6e zlCypX3uxFM32UOsqiRuoHn03545mG+uxWhTGjl/YwS9Gzf3EZuuaK2U7C7RGJxY NqVF/RoexAwLL/NcKalpjJqjah4QooUOz8sfdysZ4DROY=; Received: (qmail 22402 invoked by alias); 29 Oct 2012 16:36:35 -0000 Received: (qmail 22096 invoked by uid 22791); 29 Oct 2012 16:36:32 -0000 X-SWARE-Spam-Status: No, hits=-6.3 required=5.0 tests=AWL, BAYES_00, KHOP_RCVD_UNTRUST, KHOP_THREADED, RCVD_IN_DNSWL_HI, RP_MATCHES_RCVD, TW_OV, TW_TM 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, 29 Oct 2012 16:36:26 +0000 Received: from relay1.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 032F59FB23; Mon, 29 Oct 2012 17:36:24 +0100 (CET) Date: Mon, 29 Oct 2012 17:36:24 +0100 (CET) From: Michael Matz To: Dehao Chen Cc: Richard Biener , Eric Botcazou , GCC Patches , Jason Merrill Subject: Re: [PATCH] Fix debug info for expr and jump stmt In-Reply-To: Message-ID: References: <1945668.Pu87KJAiWx@polaris> User-Agent: Alpine 2.00 (LNX 1167 2008-08-23) MIME-Version: 1.0 X-IsSubscribed: yes Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Hi, On Mon, 29 Oct 2012, Dehao Chen wrote: > On Mon, Oct 29, 2012 at 7:17 AM, Michael Matz wrote: > > Hi, > > > > On Mon, 29 Oct 2012, Richard Biener wrote: > > > >> > Well, you merely moved the bogus code to gimple-low.c. It is bogus > >> > because you unconditionally overwrite TREE_BLOCK of all operands (and all > > Emm, then in gimple-low.c, we should probably not unconditionally > overwrite gimple_block for stmts too? That's okay, because the gimple statements were just generated during gimplification. > >> trees without block could be an issue when we extract them to some > >> other statement (then without block), and move that to some random > >> place. > > > > Even then it would be either the frontends job to set the block, or > > the the job of the pass that introduced the new expression, when there > > is a clearly sane candidate for the block. > > Please correct me if I'm wrong: front-end does not set blocks for > either stmt/expr. Yes, sorry, you're right. It's the lowering of bind expressions that tacks blocks to statements. > lower_stmt is the first place that block is set for stmt, thus I think > it should also be the first place to set block for expr. The point is, there shouldn't be any expressions left after gimplification that would receive locations/blocks. Every side-effect of computing an expression should be a separate gimple statement. The only exception should be loads for arguments of call and asm statements statements and there setting the location of those expressions to the one of the call statement itself is superfluous. > >> But the only issue should be that the stmt/expressions effective block > >> becomes a different one (the currently "active" one during expansion). > > I agree. Initially, both stmt and expressions is set to NULL block. > Whenever we update the stmt's block, we should also update the > expression's block, otherwise they will be inconsistent. No, we should not. Basically the thing to keep in mind is that the location/block of expressions after gimplification is supposed to be meaningless. The gimple statements are the anchors for locations. Everything should be made to work in this model, not by tacking locations to expression. But see below... > >> I don't see how we could end up generating too many block location > >> DIEs because of this. > > For the unittest I added, all the assembly code guarded by "if" are > should be within one lexical block, which is lock: > > LBB1 > { > asm1 > asm2 > asm3 > asm4 > .... > } > > However, because some expression are with NULL lexical block, these > assembly codes are separated by several discontinual lexical block > which is like: > > LBB1 > { > asm1 > } > asm2 > LBB2 > { > asm3 > } > asm4 > .... Hmm, I asked about specifics for this testcase. Let me find out myself... You're talking about this situation: .LBB4: .... # code for line 20 ....A .LBE4: .loc 1 22 0 movq -16(%rbp), %rax # key, tmp82 .LBB5: .... # code for line 22/23 .... .LBE5: movq -16(%rbp), %rax # key, tmp83 .LBB6: .... .LBE6 where the reload of 'key' is splitting the lexical block? This comes from _10 = key_3->rulesToParseHdl; Where the MEM_REF of *key_3 does have a location (hence locus), but no block associated: (gdb) p expand_location(((tree)0x7ffff62252d0)->exp.locus) $12 = {file = 0x7fffffffe38e "blabla.C", line = 22, column = 29, data = 0x0, sysp = false} whereas the statement has the same location, but is associated with a block: (gdb) p expand_location(curr_location ) $14 = {file = 0x7fffffffe38e "blabla.C", line = 22, column = 29, data = 0x7ffff60cf410, sysp = false} This inconsistency is indeed generated by lower_stmt, which inserts the block into this locator for the statements, but not for operands. The issue is that tree->RTL expand listens to what's stored in the EXPR_LOCATION, instead of taking the location info from curr_location (which itself comes from the currently expanded gimple statement). The patch/hack below solves this, and probably also your problem. It might introduce others, but I think we should work towards making this patch possible. I.e. follow the above layed out model, that after gimplification locators are _only_ in gimple statements, nowhere else (though I'm not sure what to do about constructs that don't generate statements, like simple global data constructors). I'm not sure how realistic reaching this goal for 4.8 is. If it isn't then I fear something along the lines of your hack is necessary. Ciao, Michael. --------------------- Index: expr.c =================================================================== --- expr.c (revision 192809) +++ expr.c (working copy) @@ -5030,7 +5030,7 @@ store_expr (tree exp, rtx target, int ca { rtx temp; rtx alt_rtl = NULL_RTX; - location_t loc = EXPR_LOCATION (exp); + location_t loc = curr_insn_location (); if (VOID_TYPE_P (TREE_TYPE (exp))) { @@ -7881,7 +7881,7 @@ expand_expr_real (tree exp, rtx target, { location_t saved_location = input_location; location_t saved_curr_loc = curr_insn_location (); - input_location = EXPR_LOCATION (exp); + input_location = curr_insn_location (); set_curr_insn_location (input_location); ret = expand_expr_real_1 (exp, target, tmode, modifier, alt_rtl); @@ -9120,7 +9120,7 @@ expand_expr_real_1 (tree exp, rtx target int ignore; tree context; bool reduce_bit_field; - location_t loc = EXPR_LOCATION (exp); + location_t loc = curr_insn_location (); struct separate_ops ops; tree treeop0, treeop1, treeop2; tree ssa_name = NULL_TREE; @@ -9481,7 +9481,7 @@ expand_expr_real_1 (tree exp, rtx target with non-BLKmode values. */ gcc_assert (GET_MODE (ret) != BLKmode); - val = build_decl (EXPR_LOCATION (exp), + val = build_decl (curr_insn_location (), VAR_DECL, NULL, TREE_TYPE (exp)); DECL_ARTIFICIAL (val) = 1; DECL_IGNORED_P (val) = 1;