From patchwork Mon Apr 15 09:47:32 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Richard Biener X-Patchwork-Id: 236549 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]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (Client CN "localhost", Issuer "www.qmailtoaster.com" (not verified)) by ozlabs.org (Postfix) with ESMTPS id D020F2C00DD for ; Mon, 15 Apr 2013 19:47:42 +1000 (EST) DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :mime-version:in-reply-to:references:date:message-id:subject :from:to:cc:content-type; q=dns; s=default; b=Fur2Mto5yOebYuwlu5 uC/D8xCCfcvIZJDGEd18bivVWGPRfObxoxVUgQcKTsxeGlQhVHtmpCo6LzM7fYlR d2R3Il3jPVndffDVbInlT4vTv1MY9CO73ssd/emHaieRgsFaBMnLlB4QKFgvk4rN RdL/qwapCuGev9xQNuYJoPVpQ= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :mime-version:in-reply-to:references:date:message-id:subject :from:to:cc:content-type; s=default; bh=GUek1B65/Fnw6xdGgC2MSABg 6Jw=; b=N3dPagi0UC4kYpE0yw1mDSbnkjgagdmuESVZzxAcL94c+UX66MXu69Cd fGhOhvVPAyYUl1L8c43Mcro+ZXPRQ1aUolTHJ+s5hl89Wp43FsHsVBop8GGd3Uka XOPKuWwcDCNhl/NcBy8yl4uWGQUJ1zzO9vLnfoVStwk4W9kxygU= Received: (qmail 26785 invoked by alias); 15 Apr 2013 09:47:35 -0000 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 Received: (qmail 26772 invoked by uid 89); 15 Apr 2013 09:47:35 -0000 X-Spam-SWARE-Status: No, score=-5.0 required=5.0 tests=AWL, BAYES_00, FREEMAIL_FROM, KHOP_RCVD_TRUST, KHOP_THREADED, RCVD_IN_DNSWL_LOW, RCVD_IN_HOSTKARMA_YE autolearn=ham version=3.3.1 Received: from mail-we0-f177.google.com (HELO mail-we0-f177.google.com) (74.125.82.177) by sourceware.org (qpsmtpd/0.84/v0.84-167-ge50287c) with ESMTP; Mon, 15 Apr 2013 09:47:34 +0000 Received: by mail-we0-f177.google.com with SMTP id o7so1358895wea.8 for ; Mon, 15 Apr 2013 02:47:32 -0700 (PDT) MIME-Version: 1.0 X-Received: by 10.180.87.170 with SMTP id az10mr10796841wib.3.1366019252252; Mon, 15 Apr 2013 02:47:32 -0700 (PDT) Received: by 10.194.56.100 with HTTP; Mon, 15 Apr 2013 02:47:32 -0700 (PDT) In-Reply-To: <2102577.TzgSJTV924@polaris> References: <13643527.5fuA5OUxPv@polaris> <2102577.TzgSJTV924@polaris> Date: Mon, 15 Apr 2013 11:47:32 +0200 Message-ID: Subject: Re: [patch] Fix ICE during RTL expansion at -O1 From: Richard Biener To: Eric Botcazou Cc: gcc-patches@gcc.gnu.org On Sun, Apr 14, 2013 at 9:46 AM, Eric Botcazou wrote: >> This is a quadratic algorithm and as such not ok. We already have >> aliasing_component_refs_p in tree-ssa-alias.c which is supposed to be >> the non-quadratic replacement. It's not used via decl_refs_may_alias_p, >> so that may be the thing to fix. > > aliasing_component_refs_p isn't powerful enough, it eliminates the quadratic > aspect by assuming that all offsets are constants, so it misses cases like > (*p)[i].f1 vs a[j].f2. Moreover it assumes TBAA and we don't need it here. Note that looking at the access path _is_ assuming TBAA constraints as soon as the base objects are not the same (in the above case '*p' and 'a' are not the same and p could alias a in a way that all f1 and f2 overlap). > I can rewrite nonoverlapping_component_refs_of_decl_p to make it non-quadratic > and catch the same cases I think, patch attached (without the vect testsuite > adjustments, but they are still needed). > >> nonoverlapping_component_refs_of_decl_p on RTL should go - in fact >> we do call the tree oracle from all its callers so we only ever do redundant >> work (after your proposed patch even more so). > > Not clear if the tree oracle can catch the above case with *p and a, but, yes, > nonoverlapping_component_refs_p should go in the long term. > > > * alias.c (nonoverlapping_component_refs_p): Protect again LTO quirk. > * tree-ssa-alias.c (nonoverlapping_component_refs_of_decl_p): New. > (decl_refs_may_alias_p): Add REF1 and REF2 parameters. > Use nonoverlapping_component_refs_of_decl_p to disambiguate component > references. > (refs_may_alias_p_1): Adjust call to decl_refs_may_alias_p. > * tree-streamer.c (record_common_node): Adjust reference in comment. without actually removing nonoverlapping_component_refs_p it still applies to both... Can you port the non-quadratic algorithm to the RTL nonoverlapping_component_refs_p first? The core code should exactly be the same ... (and shared, and only called once from the RTL oracle). + /* Pop the stacks in parallel and examine the COMPONENT_REFs of the same + rank. This is sufficient because you cannot reference several fields + at a time (unlike for arrays with slices), unless you're in a union, + in which case the return value will be false in any case. */ + while (true) I don't understand the "reference several fields" comment. Because I can clearly have aggregate copies of RECORD_TYPE. Can you try do elaborate more on why the algorithm should be sufficient to catch all cases? You could enhance it to not require + /* We must have the same base DECL. */ + gcc_assert (ref1 == ref2); for MEM_REF bases under the same conditions like aliasing_component_refs_p, that is if the MEM_REF isn't view-converting. That said, if the bases are the same DECLs then indeed you do not rely on TBAA. The RTL nonoverlapping_component_refs_p does not disable itself properly for pointer based accesses that might be view-converted / aliased accesses (a simple testcase with ref-all pointers properly offsetted to alias two adjacent fields may be enough to show that). Also with your patch enhanced like I suggest we should be able to remove aliasing_component_refs_p as well, no? Thanks, Richard. > > -- > Eric Botcazou Index: alias.c =================================================================== --- alias.c (revision 197926) +++ alias.c (working copy) @@ -2232,8 +2232,11 @@ nonoverlapping_component_refs_p (const_r found: /* If we're left with accessing different fields of a structure, then no - possible overlap, unless they are both bitfields. */ - if (TREE_CODE (typex) == RECORD_TYPE && fieldx != fieldy) + possible overlap, unless they are both bitfields. + ??? Pointer inequality is too fragile in the LTO compiler. */ + if (TREE_CODE (typex) == RECORD_TYPE + && fieldx != fieldy + && DECL_NAME (fieldx) != DECL_NAME (fieldy)) this, if at all, should go in with a separate patch and a testcase. And I think it should _not_ go in. Instead, as the case passes if (typex == typey) goto found; earlier you should assert that DECL_CONTEXT (fieldx) == DECL_CONTEXT (fieldy) == typex == typey here. Note that fails of this test are expected even in the non-LTO case because I cannot find any IL verification that would verify that for a COMPONENT_REF TREE_TYPE (TREE_OPERAND (cr, 0)) == DECL_CONTEXT (TREE_OPERAND (cr, 1)) (due to sharing of the FIELD_DECL chain between different type variants the check will fail for all non-main-variants I think, so refining it to look at the main variant is probably advised). Otoh... + /* ??? We cannot simply use the type of operand #0 of the refs here + as the Fortran compiler smuggles type punning into COMPONENT_REFs + for common blocks instead of using unions like everyone else. */ + tree type1 = TYPE_MAIN_VARIANT (DECL_CONTEXT (field1)); + tree type2 = TYPE_MAIN_VARIANT (DECL_CONTEXT (field2)); + + if (type1 != type2 || TREE_CODE (type1) != RECORD_TYPE) + goto may_overlap; + + /* ??? Pointer inequality is too fragile in the LTO compiler. */ + if (field1 != field2 && DECL_NAME (field1) != DECL_NAME (field2)) this suggests you are seeing multiple FIELD_DECLs for the same field in the _same_ FIELD_DECL chain ...?! Are you sure this happens with GCC 4.8? There were some fixes in that area in the LTO type merging code. Index: tree-streamer.c =================================================================== --- tree-streamer.c (revision 197926) +++ tree-streamer.c (working copy) @@ -267,10 +267,9 @@ record_common_node (struct streamer_tree /* The FIELD_DECLs of structures should be shared, so that every COMPONENT_REF uses the same tree node when referencing a field. Pointer equality between FIELD_DECLs is used by the alias - machinery to compute overlapping memory references (See - nonoverlapping_component_refs_p). */ - tree f; - for (f = TYPE_FIELDS (node); f; f = TREE_CHAIN (f)) + machinery to compute overlapping component references (see + nonoverlapping_component_refs_of_decl_p). */ + for (tree f = TYPE_FIELDS (node); f; f = TREE_CHAIN (f)) record_common_node (cache, f); } }