From patchwork Mon Jun 14 22:44:04 2010 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Bernd Schmidt X-Patchwork-Id: 55594 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 650F9B7D61 for ; Tue, 15 Jun 2010 08:44:02 +1000 (EST) Received: (qmail 7534 invoked by alias); 14 Jun 2010 22:44:00 -0000 Received: (qmail 7525 invoked by uid 22791); 14 Jun 2010 22:43:59 -0000 X-SWARE-Spam-Status: No, hits=-1.9 required=5.0 tests=AWL, BAYES_00, T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mail.codesourcery.com (HELO mail.codesourcery.com) (38.113.113.100) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Mon, 14 Jun 2010 22:43:55 +0000 Received: (qmail 17828 invoked from network); 14 Jun 2010 22:43:53 -0000 Received: from unknown (HELO ?84.152.218.187?) (bernds@127.0.0.2) by mail.codesourcery.com with ESMTPA; 14 Jun 2010 22:43:53 -0000 Message-ID: <4C16B0B4.6080604@codesourcery.com> Date: Tue, 15 Jun 2010 00:44:04 +0200 From: Bernd Schmidt User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1.9) Gecko/20100425 Thunderbird/3.0.4 MIME-Version: 1.0 To: GCC Patches CC: Joern Rennecke Subject: Followup for reg_equiv_invariant patch: Fix PR39871 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 For flag_pic, reload currently disables the reg_equiv_invariant machinary, which leads to poor code generation in PR39871 (a variant of PR42500, 42502 and 40615 which were fixed by my earlier patch). The problematic if statement looks like this: if (! function_invariant_p (x) || ! flag_pic /* A function invariant is often CONSTANT_P but may include a register. We promise to only pass CONSTANT_P objects to LEGITIMATE_PIC_OPERAND_P. */ || (CONSTANT_P (x) && LEGITIMATE_PIC_OPERAND_P (x))) It has a bit of history. Originally, we only supported REG_EQUIV for constants, and it looked like this: if (note #ifdef LEGITIMATE_PIC_OPERAND_P && (! CONSTANT_P (XEXP (note, 0)) || ! flag_pic || LEGITIMATE_PIC_OPERAND_P (XEXP (note, 0))) #endif ) This simply avoids using constants that aren't LEGITIMATE_PIC_OPERAND_P. Then, Joern added the machinery that allows us to process equivalences for things that are function_invariant_p, and I think this is where the mistake happened: #ifdef LEGITIMATE_PIC_OPERAND_P - && (! CONSTANT_P (XEXP (note, 0)) || ! flag_pic + && (! function_invariant_p (XEXP (note, 0)) + || ! flag_pic || LEGITIMATE_PIC_OPERAND_P (XEXP (note, 0))) #endif This looks like an error - the patch replaces several occurrences of CONSTANT_P with function_invariant_p in order to enable the extra optimization, but here I think the test should simply have been left alone; we deal with the various cases of equivalences inside this if statement. This also introduced a new bug: later on, someone else noticed that this code could now pass non-constant objects to LEGITIMATE_PIC_OPERAND_P, and added the extra CONSTANT_P check around that test, which gives us the current version of the code. The patch below simply restores this to essentially the original form. Regression tested on arm-none-linux-gnueabi with my usual set of multilibs (also bootstrapped and regression tested on i686-linux last week IIRC, will retest before checking in). Joern, any comments - do you recall any reason why this change would have been intentional? Bernd PR rtl-optimization/39871 * reload1.c (init_eliminable_invariants): For flag_pic, disable equivalences only for constants that aren't LEGITIMATE_PIC_OPERAND_P. * ira.c (find_reg_equiv_invariant_const): Likewise. Index: reload1.c =================================================================== --- reload1.c (revision 160663) +++ reload1.c (working copy) @@ -4151,13 +4151,10 @@ init_eliminable_invariants (rtx first, b if (i <= LAST_VIRTUAL_REGISTER) continue; - if (! function_invariant_p (x) - || ! flag_pic - /* A function invariant is often CONSTANT_P but may - include a register. We promise to only pass - CONSTANT_P objects to LEGITIMATE_PIC_OPERAND_P. */ - || (CONSTANT_P (x) - && LEGITIMATE_PIC_OPERAND_P (x))) + /* If flag_pic and we have constant, verify it's legitimate. */ + if (!CONSTANT_P (x) + || !flag_pic + || LEGITIMATE_PIC_OPERAND_P (x)) { /* It can happen that a REG_EQUIV note contains a MEM that is not a legitimate memory operand. As later Index: ira.c =================================================================== --- ira.c (revision 160663) +++ ira.c (working copy) @@ -1586,12 +1586,12 @@ find_reg_equiv_invariant_const (void) x = XEXP (note, 0); - if (! function_invariant_p (x) + if (! CONSTANT_P (x) || ! flag_pic /* A function invariant is often CONSTANT_P but may include a register. We promise to only pass CONSTANT_P objects to LEGITIMATE_PIC_OPERAND_P. */ - || (CONSTANT_P (x) && LEGITIMATE_PIC_OPERAND_P (x))) + || LEGITIMATE_PIC_OPERAND_P (x)) { /* It can happen that a REG_EQUIV note contains a MEM that is not a legitimate memory operand. As later