From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 933 invoked by alias); 19 Oct 2004 21:27:55 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 909 invoked from network); 19 Oct 2004 21:27:53 -0000 Received: from unknown (HELO x93.infopact.nl) (212.29.160.93) by sourceware.org with SMTP; 19 Oct 2004 21:27:53 -0000 Received: from steven.lr-s.tudelft.nl (70-90.ipact.nl [82.210.90.70]) by x93.infopact.nl (8.12.10/8.12.10) with ESMTP id i9JLRqAw003047; Tue, 19 Oct 2004 23:27:52 +0200 From: Steven Bosscher Organization: SUSE Labs To: law@redhat.com Subject: Re: Fix a tcb crash and a potential bug on mainline Date: Tue, 19 Oct 2004 21:29:00 -0000 User-Agent: KMail/1.5.4 Cc: gcc-patches@gcc.gnu.org, dnovillo@redhat.com, amacleod@redhat.com References: <200410182303.52997.stevenb@suse.de> <1098218905.2915.123.camel@localhost.localdomain> In-Reply-To: <1098218905.2915.123.camel@localhost.localdomain> MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Content-Disposition: inline Message-Id: <200410192328.28252.stevenb@suse.de> X-CanItPRO-Stream: NoScan X-Spam-Score: undef - spam-scanning disabled X-Canit-Stats-ID: 1166984 - 8b61f2259502 X-Scanned-By: CanIt (www . canit . ca) X-SW-Source: 2004-10/txt/msg01627.txt.bz2 On Tuesday 19 October 2004 22:48, Jeffrey A Law wrote: > On Mon, 2004-10-18 at 15:03, Steven Bosscher wrote: > > Later on cleanup_control_flow() tries to do its thing on basic block 4. > > It tries to fold "(c_13 != 0B)", but c_13 is on the free list, so it > > does not have a TREE_TYPE, so fold crashes. > > Hmm. I wonder if this is the same bug Andrew Pinski was trying to > fix by not clearing TREE_TYPE. Your description matches a lot of > the tidbits Andrew mentioned. Yes, it is. > > I think the root cause of this bug is that c_13 is put on the free list, > > and I think its SSA_NAME_DEF_STMT should be set to the empty statement, > > because remove_phi_arg_num() cannot know if there are any immediate > > uses of it still left. > > ??? Putting c_13 on the free list is the right thing to do. I believe it is not. It is *wrong* to put an SSA_NAME on the free list unless you're absolutely positively sure that the thing is not used anywhere in the code. The free list is for SSA names that are available for recycling. An SSA_NAME that still appears in the code, even if it is in unreachable code, is *not* elligible for recycling until that unreachable code is actually removed. I really don't understand how you can think this is the right thing to do, it's the most broken idea I've seen in a long time. > I'm not > sure why you think setting SSA_NAME_DEF_STMT to NULL is particularly > interesting though. Whether or not the name has any uses left in the > IL isn't something we use to determine whether or not to set > SSA_NAME_DEF_STMT to any particular value IIRC. Not usually. But this case is special. Normally we don't just throw away a statement, we first update the code or we delete entire blocks of code. In this particular case we're only deleting a PHI node and we don't know if the block we're deleting the PHI from is unreachable (we could, but it's not the business of remove_phi_arg_num() to remove unreachable blocks). So conceptually the PHI_RESULT of the deleted PHI becomes undefined. But it is not dead until all its uses are also removed. We express "undefined-ness" by setting the SSA_NAME_DEF_STMT to the empty statement. > > I don't know if making that change could have > > some unforseen effects, so I decided to re-hide the the bug by just not > > looking at unreachable blocks in cleanup_control_flow(). > > I'm not sure this is sufficient to hide the bug well enough though. Me neither. But I am fairly sure that making the PHI_RESULT undefined, instead of putting it on the free list, will not just hide but actually *fix* it. I would have posted a patch to do that if I had had more time and was less unsure about unexpected effects of such a change. I might still give it a try. > > This may be something Andrew may want to look into as part > > of his immediate uses work?? > > It's something we've pondered and talked about a little. Basically > with the immediate use work we would know where all the uses are > and we could probably arrange to delete those uses. How would you delete those uses? You don't know if those statements are dead, so you really can't do that. What I mean with this is that perhaps we should not put the SSA_NAME on the free list if there still are immediate uses. On Andrew's branch that is very easy to do, if we decide to fix the bug this way. > > This fix is completely untested at the moment, but it's quite obvious > > and it fixes the ICE I was seeing. Still, I am a bit surprised that we > > have not been in trouble over this bug before - perhaps we should still > > fix the remove_phi_arg_num() issue instead of papering it over this way? > > I'm not sure what you mean by fix remove_phi_arg_num I mean to fix it by making it set the SSA_NAME_DEF_STMT of PHIs that it is deleting to the empty statement... > except to compute > the immediate uses of the result and delete those statements, which > in turn can cause other statements to get deleted. Basically it turns > into immediate use based DCE. Roughly the same thing we'd end up > doing if we tried to do integrated dead code removal using > immediate uses. ...and this is what I don't mean and don't want. (Not for the CFG cleanup anyway. Perhaps doing immediate uses based DCE is a good idea for the mythical "fast DCE". LLVM used this algorithm last time I looked. I don't know which is faster in practice, classic DCE like we have now, or immediate uses based DCE. Something we can try on Andrews branch. ) > Alternately we could split up cleanup_control_expr_graph into two > phases. The first identifies all the COND_EXPRs and SWITCH_EXPRs > which need updating, but does not do any edge removal. Once we > have done our full IL scan, we go back to the nodes where we know > which outgoing edge will be taken and proceed to remove the > COND_EXPR/SWITCH_EXPR and the untaken edges. Talk about killing a bug with a sledge hammer :-) Take a step back, and consider what is really happening: Conceptually, after removing the PHI node, the PHI_RESULT c_13 simply becomes an SSA_NAME without a defining statement. The obviously correct way to handle this is by treating it just like we treat any other SSA name without defining statement: Setting SSA_NAME_DEF_STMT to the empty statement. The whole infrastructure already knows how to deal with such SSA_NAMEs, so except a change in remove_phi_arg_num() it would probably not need any changes anywhere else. At least that's what I hope, because I'm going to try this approach now... Gr. Steven