From patchwork Sun Aug 8 14:37:42 2010 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Richard Sandiford X-Patchwork-Id: 61213 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 361AAB6EEE for ; Mon, 9 Aug 2010 00:37:59 +1000 (EST) Received: (qmail 21450 invoked by alias); 8 Aug 2010 14:37:57 -0000 Received: (qmail 21438 invoked by uid 22791); 8 Aug 2010 14:37:56 -0000 X-SWARE-Spam-Status: No, hits=-1.7 required=5.0 tests=AWL, BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, FREEMAIL_FROM, T_TO_NO_BRKTS_FREEMAIL X-Spam-Check-By: sourceware.org Received: from mail-wy0-f175.google.com (HELO mail-wy0-f175.google.com) (74.125.82.175) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Sun, 08 Aug 2010 14:37:51 +0000 Received: by wyb38 with SMTP id 38so9901367wyb.20 for ; Sun, 08 Aug 2010 07:37:48 -0700 (PDT) Received: by 10.227.155.20 with SMTP id q20mr5322296wbw.98.1281278268381; Sun, 08 Aug 2010 07:37:48 -0700 (PDT) Received: from localhost (rsandifo.gotadsl.co.uk [82.133.89.107]) by mx.google.com with ESMTPS id h3sm3398847wbb.21.2010.08.08.07.37.46 (version=TLSv1/SSLv3 cipher=RC4-MD5); Sun, 08 Aug 2010 07:37:47 -0700 (PDT) From: Richard Sandiford To: gcc-patches@gcc.gnu.org Mail-Followup-To: gcc-patches@gcc.gnu.org, vmakarov@redhat.com, rdsandiford@googlemail.com Cc: vmakarov@redhat.com Subject: PR 43358: out-of-bounds array access in IRA User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/23.1 (gnu/linux) Date: Sun, 08 Aug 2010 15:37:42 +0100 Message-ID: <871va9qifd.fsf@firetop.home> 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 PR 43358 is about an ICE when freeing memory in IRA. The testcase in that PR was derived from linux, but the same thing can be seem with gcc.c-torture/execute/pr23135.c when compiled with -O or above on mips64-linux-gnu/-mabi=32. The immediate cause of the ICE is a write into index -1 of an ALLOCNO_CONFLICT_HARD_REG_COSTS array, which clobbers the pool id. But I think the problem is a little deeper than that. The code in question is part of the single-register-class handling: if (GET_CODE (operand) == SUBREG) operand = SUBREG_REG (operand); if (REG_P (operand) && (regno = REGNO (operand)) >= FIRST_PSEUDO_REGISTER) { enum machine_mode mode; enum reg_class cover_class; operand_a = ira_curr_regno_allocno_map[regno]; mode = ALLOCNO_MODE (operand_a); cover_class = ALLOCNO_COVER_CLASS (operand_a); if (ira_class_subset_p[cl][cover_class] && ira_class_hard_regs_num[cl] != 0 && (ira_class_hard_reg_index[cover_class] [ira_class_hard_regs[cl][0]]) >= 0 && reg_class_size[cl] <= (unsigned) CLASS_MAX_NREGS (cl, mode)) { int i, size; cost = (freq * (in_p ? ira_get_register_move_cost (mode, cover_class, cl) : ira_get_register_move_cost (mode, cl, cover_class))); ira_allocate_and_set_costs (&ALLOCNO_CONFLICT_HARD_REG_COSTS (operand_a), cover_class, 0); size = ira_reg_class_nregs[cover_class][mode]; for (i = 0; i < size; i++) ALLOCNO_CONFLICT_HARD_REG_COSTS (operand_a) [ira_class_hard_reg_index [cover_class][ira_class_hard_regs[cl][i]]] -= cost; } } What we have in the testcase is an SImode subreg of a DImode pseudo. The code has correctly realised that the pattern requires the SImode subreg to be in a single register class (MD1_REG), and is trying to reflect that restriction in the costs of the DImode allocno. Conceptually, this is exactly what we want to happen in the testcase. However, the loop is iterating over the size of the _allocno_, and expecting every register to belong to the single-register class (the ira_class_hard_regs[cl][i] expression). Because the allocno is wider than the operand, that assumption doesn't hold. The preferred register for the allocno (as opposed to the operand) is outside MD1_REG, but is still in the cover class. Which brings me to a question. If it takes several consecutive registers to store a particular mode, am I right in thinking that it is the ALLOCNO_CONFLICT_HARD_REG_COSTS of the _first_ of those registers (and only the first of those registers) that matters? For example. is the cost of storing a quadword value in registers (4, 5, 6, 7) reflected solely in ALLOCNO_CONFLICT_HARD_REG_COSTS (a)[4]? That looks to be how the copy costs for hard registers works, and it seems to make conceptual sense. Adding the cost for every register in the group would make an overlapping register group like (5, 6, 7, 8) seem better than an independent group like (8, 9, 10, 11), whereas I imagine the former would often be worse (harder reloads). If the answer is "no, you're wrong", then the patch is wrong too. But if the answer is "yes", then I'm puzzled why the code above is looping at all. We've already established by this point (or at least, are already confident enough to assume) that the operand needs a single register. So why not just work out what that register is, and try to calculate the corresponding register in the allocno's mode? That's what the patch below does. simplify_subreg_regno deals with (is supposed to deal with) cases where the subreg is invalid, such as if the register number is out of bounds, or if the mode change is rejected by the target. And we do of course still need to check that the result belongs to the allocno's class. I checked that the lower costs are still applied for this testcase and for others like it. I also compared the testsuite output for -O2 before and after the patch, both on mips64-linux-gnu and x86_64-linux-gnu. There were three changes in the mips64-linux-gnu output and no changes in the x86_64-linux-gnu output. Two of the three changes were significant improvements; they were cases where we used to copy a GPR into LO and back again for no reason. The other change was simply a renumbering of GPRs. I realise this change probably conflicts with the ira improvements branch, so I'm happy to sit on it until those changes go in. But I think the same register-selection logic should apply regardless of whether we're using cover classes. Either way, I'd like to apply the patch to 4.5 branch at some point, since this is a regression from 4.4. Richard gcc/ PR rtl-optimization/43358 * ira-lives.c (process_single_reg_class_operands): Adjust the costs of a single hard register, using simplify_subreg_regno to decide what that register should be. Index: gcc/ira-lives.c =================================================================== --- gcc/ira-lives.c 2010-08-08 10:35:06.000000000 +0100 +++ gcc/ira-lives.c 2010-08-08 10:35:08.000000000 +0100 @@ -896,7 +896,7 @@ ira_implicitly_set_insn_hard_regs (HARD_ static void process_single_reg_class_operands (bool in_p, int freq) { - int i, regno, cost; + int i, regno; unsigned int px; enum reg_class cl; rtx operand; @@ -923,32 +923,46 @@ process_single_reg_class_operands (bool if (REG_P (operand) && (regno = REGNO (operand)) >= FIRST_PSEUDO_REGISTER) { - enum machine_mode mode; enum reg_class cover_class; operand_a = ira_curr_regno_allocno_map[regno]; - mode = ALLOCNO_MODE (operand_a); cover_class = ALLOCNO_COVER_CLASS (operand_a); if (ira_class_subset_p[cl][cover_class] - && ira_class_hard_regs_num[cl] != 0 - && (ira_class_hard_reg_index[cover_class] - [ira_class_hard_regs[cl][0]]) >= 0 - && reg_class_size[cl] <= (unsigned) CLASS_MAX_NREGS (cl, mode)) + && ira_class_hard_regs_num[cl] != 0) { - int i, size; - cost - = (freq - * (in_p - ? ira_get_register_move_cost (mode, cover_class, cl) - : ira_get_register_move_cost (mode, cl, cover_class))); - ira_allocate_and_set_costs - (&ALLOCNO_CONFLICT_HARD_REG_COSTS (operand_a), cover_class, 0); - size = ira_reg_class_nregs[cover_class][mode]; - for (i = 0; i < size; i++) - ALLOCNO_CONFLICT_HARD_REG_COSTS (operand_a) - [ira_class_hard_reg_index - [cover_class][ira_class_hard_regs[cl][i]]] - -= cost; + /* View the desired allocation of OPERAND as: + + (REG:YMODE YREGNO), + + a simplification of: + + (subreg:YMODE (reg:XMODE XREGNO) OFFSET). */ + enum machine_mode ymode, xmode; + int xregno, yregno; + HOST_WIDE_INT offset; + + xmode = recog_data.operand_mode[i]; + xregno = ira_class_hard_regs[cl][0]; + ymode = ALLOCNO_MODE (operand_a); + offset = subreg_lowpart_offset (ymode, xmode); + yregno = simplify_subreg_regno (xregno, xmode, offset, ymode); + if (yregno >= 0 + && ira_class_hard_reg_index[cover_class][yregno] >= 0) + { + int cost; + + ira_allocate_and_set_costs + (&ALLOCNO_CONFLICT_HARD_REG_COSTS (operand_a), + cover_class, 0); + cost + = (freq + * (in_p + ? ira_get_register_move_cost (xmode, cover_class, cl) + : ira_get_register_move_cost (xmode, cl, + cover_class))); + ALLOCNO_CONFLICT_HARD_REG_COSTS (operand_a) + [ira_class_hard_reg_index[cover_class][yregno]] -= cost; + } } }