From patchwork Mon Jun 28 10:58:19 2010 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Richard Biener X-Patchwork-Id: 57130 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 A1287B6EF7 for ; Mon, 28 Jun 2010 20:58:31 +1000 (EST) Received: (qmail 17816 invoked by alias); 28 Jun 2010 10:58:29 -0000 Received: (qmail 17804 invoked by uid 22791); 28 Jun 2010 10:58:27 -0000 X-SWARE-Spam-Status: No, hits=-5.9 required=5.0 tests=AWL, BAYES_00, RCVD_IN_DNSWL_HI, T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from cantor.suse.de (HELO mx1.suse.de) (195.135.220.2) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Mon, 28 Jun 2010 10:58:23 +0000 Received: from relay1.suse.de (charybdis-ext.suse.de [195.135.221.2]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.suse.de (Postfix) with ESMTP id 28A196CB00; Mon, 28 Jun 2010 12:58:20 +0200 (CEST) Date: Mon, 28 Jun 2010 12:58:19 +0200 (CEST) From: Richard Guenther To: Paolo Bonzini Cc: gcc-patches@gcc.gnu.org Subject: Re: [PATCH] mem-ref2 merge, core patch In-Reply-To: <4C287738.5000508@gnu.org> Message-ID: References: <4C287738.5000508@gnu.org> User-Agent: Alpine 2.00 (LNX 1167 2008-08-23) MIME-Version: 1.0 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 On Mon, 28 Jun 2010, Paolo Bonzini wrote: > On 06/25/2010 01:47 PM, Richard Guenther wrote: > > + if (integer_zerop (TREE_OPERAND (node, 1)) > > + /* Same pointer types, but ignoring POINTER_TYPE vs. > > + REFERENCE_TYPE. */ > > + && (TREE_TYPE (TREE_TYPE (TREE_OPERAND (node, 0))) > > + == TREE_TYPE (TREE_TYPE (TREE_OPERAND (node, 1)))) > > + && (TYPE_MODE (TREE_TYPE (TREE_OPERAND (node, 0))) > > + == TYPE_MODE (TREE_TYPE (TREE_OPERAND (node, 1)))) > > + && (TYPE_REF_CAN_ALIAS_ALL (TREE_TYPE (TREE_OPERAND (node, 0))) > > + == TYPE_REF_CAN_ALIAS_ALL (TREE_TYPE (TREE_OPERAND (node, > > 1)))) > > + && (TYPE_QUALS (TREE_TYPE (TREE_OPERAND (node, 0))) > > + == TYPE_QUALS (TREE_TYPE (TREE_OPERAND (node, 1)))) > > + /* Same value types ignoring qualifiers. */ > > + && (TYPE_MAIN_VARIANT (TREE_TYPE (node)) > > + == TYPE_MAIN_VARIANT > > + (TREE_TYPE (TREE_TYPE (TREE_OPERAND (node, 1)))))) > > + { > > + if (TREE_CODE (TREE_OPERAND (node, 0)) != ADDR_EXPR) > > + { > > + pp_string (buffer, "*"); > > + dump_generic_node (buffer, TREE_OPERAND (node, 0), > > + spc, flags, false); > > + } > > Maybe avoid the magic when detailed dumping is active? The magic is mostly to make the testsuite part of the patch smaller, which also means that if we change the above with -details some testcases may start failing. I'll check, the suggestion is good I think (so if the resulting change in testresults is small I'll probably go for it). > What exactly are the rules for having an INDIRECT_REF or a chain of > handled_component_p instead of a MEM_REF? INDIRECT_REF is not allowed in gimple. You can have bare decls or MEM_REF, and both can be wrapped inside a handled_component_p chain. (There also still exist ALIGN_INDIRECT_REF and MISALIGNED_INDIRECT_REF created by the vectorizer - I will do followup patches to remove those as well) Basically MEM_REF can appear wherever an INDIRECT_REF or a decl could previously apper. > Would it make sense to change the other INDIRECT_REF_P types into flags on > MEM_REF? INDIRECT_REF_P will be no longer necessary on gimple after I fixed up the vectorizer to not use ALIGN_INDIRECT_REF and MISALIGNED_INDIRECT_REF. Where it currently appears it is solely because of those two (I didn't bother to change it to tests for ALIGN_INDIRECT_REF and MISALIGNED_INDIRECT_REF excluding INDIRECT_REF). > Can you expand on the (several) "#if 0/FIXME" comments? They should have been fixed by followup patches (I just noticed them while writing the changelog). Remaining FIXMEs are + /* ??? FIXME. We should always fold these. */ + && !CONSTANT_CLASS_P (TREE_OPERAND (TREE_OPERAND (expr, 0), 0))) Is not true. I removed the FIXME in a followup. Several ???s just mark possible improvements (or rather highlights places where we trade optimization for correctness, compared to before MEM_REF). Like in tree-ssa-alias.c. There are probably issues in the RTL/expansion parts of the patch, mostly regarding to possibly not running into optimized code-paths. Expansion of assignments is really an interesting mystery. So I'd appreciate 2nd looks in that area (though I think all improvements or fixes can be done as a followup after the merge). @@ -3269,13 +3272,16 @@ gimplify_init_ctor_preeval_1 (tree *tp, /* If the constructor component is a call, determine if it can hide a - potential overlap with the lhs through an INDIRECT_REF like above. */ + potential overlap with the lhs through an INDIRECT_REF like above. + ??? Ugh - this is completely broken. In fact this whole analysis + doesn't look conservative. */ if (TREE_CODE (t) == CALL_EXPR) well - this comment just hints at that the following code looks broken to me. It falls afoul to the same wrong assumptions that made escape analysis in 4.3 and before completely broken. I think that were all FIXME/???s in the core patch. Thanks, Richard. Index: gcc/dwarf2out.c =================================================================== --- gcc/dwarf2out.c (.../trunk) (revision 161367) +++ gcc/dwarf2out.c (.../branches/mem-ref2) (revision 161369) @@ -15159,6 +15159,11 @@ loc_list_from_tree (tree loc, int want_a } break; + case MEM_REF: + /* ??? FIXME. */ + if (!integer_zerop (TREE_OPERAND (loc, 1))) + return 0; + /* Fallthru. */ I have no idea what to do here (no clue about dwarf). Similar for the expand_debug_expr case.