From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 25977 invoked by alias); 12 Jul 2004 13:38:23 -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 25922 invoked from network); 12 Jul 2004 13:38:20 -0000 Received: from unknown (HELO mx1.redhat.com) (66.187.233.31) by sourceware.org with SMTP; 12 Jul 2004 13:38:20 -0000 Received: from int-mx1.corp.redhat.com (int-mx1.corp.redhat.com [172.16.52.254]) by mx1.redhat.com (8.12.10/8.12.10) with ESMTP id i6CDcEe1012071; Mon, 12 Jul 2004 09:38:14 -0400 Received: from localhost (mail@vpnuser2.surrey.redhat.com [172.16.9.2]) by int-mx1.corp.redhat.com (8.11.6/8.11.6) with ESMTP id i6CDcC013289; Mon, 12 Jul 2004 09:38:12 -0400 Received: from rsandifo by localhost with local (Exim 3.35 #1) id 1Bk10V-0006G2-00; Mon, 12 Jul 2004 14:38:11 +0100 To: Alexandre Oliva Cc: Kazu Hirata , gcc-patches@gcc.gnu.org Subject: Re: add h8sx support to h8300 References: <20040621.102356.74724063.kazu@cs.umass.edu> <87fz83f456.fsf@redhat.com> <87zn6aevpa.fsf@redhat.com> <873c42e8rb.fsf@redhat.com> From: Richard Sandiford Date: Mon, 12 Jul 2004 20:14:00 -0000 Message-ID: <87r7rh8hx8.fsf@redhat.com> User-Agent: Gnus/5.1006 (Gnus v5.10.6) Emacs/21.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii X-SW-Source: 2004-07/txt/msg01269.txt.bz2 Alexandre Oliva writes: > Maybe you can still get the problem with dg/builtin-apply2.c? I > remember it failed to compile because of some movmd-related problems. Thanks for the pointer. builtin-apply2.c does indeed fail without your change. It turns out that the problem was in: if (!regs_ever_live[FP_REG]) return NO_REGS; When I wrote the code, there was no such thing as HFP_REG, and FP_REG was 6. As explained (poorly, I expect ;) in the comment above the function, the idea was to return NO_REGS if er6 wasn't yet live. So with the new HFP_REG/FP_REG distinction, the code should instead read: if (!regs_ever_live[HFP_REG]) return NO_REGS; FWIW, the patch below puts back the old code, but with the additional FP_REG->HFP_REG change, and builtin-apply2.c now compiles. > Anyhow, whether the testcase passes with your patch or not is not very > relevant. Using !D at that point is wrong, for the reasons explained > in the comments. > > ! prevents reload from even considering an alternative. So, given > `d,!D', if it couldn't satisfy the constraint in local or global > alloc, it will require r6 (or, worse, NO_REGS, if `d' happens to > expand differently) during reload. NO_REGS will obviously be > impossible to satisfy, and r6 may be impossible to satisfy if the > frame pointer is found to be needed during reload (this happens in > builtin-apply2). See above for the builtin-apply2 bit. But wrt the first sentence, the idea is precisely to prevent reload from considering the 'D' alternative if er6 is an allocatable register. I.e., when the instruction is being reloaded, there are two cases: (a) er6 is allocatable. Then: 'd' maps to DESTINATION_REGS 'D' maps to GENERAL_REGS but we absolutely want reload to pick the first alternative if it can, even if that means spilling into and out of er6. If it doesn't choose the first alternative, we have to spill er6 ourselves anyway. (b) er6 is not allocatable. Then: 'd' maps to NO_REGS 'D' maps to GENERAL_REGS Reload will ignore the first alternative since, like you say, NO_REGS can't be satisfied. It is forced to use the second alternative instead. The built-in-setjmp.c -O3 -fomit-frame-pointer failure (which, to remind anyone else reading, is there with and without the patch below) does show up a problem. And this might well be the problem you were trying to explain above. (Sorry if so! I couldn't quite follow what you were saying.) We have the following sequence of events, all in the main reload() loop: 1. We pick a set of reloads each instruction. In the case of movmd, we pick DESTINATION_REGS (the class containing only the frame pointer), since the frame pointer is still allocatable at this point. 2. We discover that the function now needs a frame pointer. 3. We discover that the frame pointer was previously needed as a spill register and that we must therefore recompute the reloads (something_changed = 1). 4. We nevertheless proceed to select_reload_regs() for the old set of reloads. This leads to the movmd spill failure since there are no longer any allocatable registers in DESTINATION_REGS. This probably shows my ignorance, but I wasn't planning on (4). I was expecting the reload loop to repeat straight away once it realised that the old reloads weren't viable. So it seems you just can't have a register class that includes only the frame pointer. As a proof of concept: Index: reload1.c =================================================================== RCS file: /cvs/gcc/gcc/gcc/reload1.c,v retrieving revision 1.441 diff -c -p -F^\([(a-zA-Z0-9_]\|#define\) -r1.441 reload1.c *** reload1.c 9 Jul 2004 03:29:34 -0000 1.441 --- reload1.c 12 Jul 2004 13:32:01 -0000 *************** reload (rtx first, int global) *** 1007,1012 **** --- 1007,1014 ---- something_changed = 1; } } + if (something_changed) + continue; select_reload_regs (); if (failure) bypasses select_reload_regs() when the set of eliminable registers has changed. It fixes the testcase, but I doubt it's right. ;) Richard Index: config/h8300/h8300.c =================================================================== RCS file: /cvs/gcc/gcc/gcc/config/h8300/h8300.c,v retrieving revision 1.290 diff -c -p -F^\([(a-zA-Z0-9_]\|#define\) -r1.290 h8300.c *** config/h8300/h8300.c 8 Jul 2004 03:40:31 -0000 1.290 --- config/h8300/h8300.c 9 Jul 2004 09:34:24 -0000 *************** h8300_init_once (void) *** 454,469 **** before reload so that register allocator will pick the second alternative. ! - we would like 'D' to be be NO_REGS when the frame pointer isn't ! live, but we the frame pointer may turn out to be needed after ! we start reload, and then we may have already decided we don't ! have a choice, so we can't do that. Forcing the register ! allocator to use er6 if possible might produce better code for ! small functions: it's more efficient to save and restore er6 in ! the prologue & epilogue than to do it in a define_split. ! Hopefully disparaging 'D' will have a similar effect, without ! forcing a reload failure if the frame pointer is found to be ! needed too late. */ enum reg_class h8300_reg_class_from_letter (int c) --- 454,465 ---- before reload so that register allocator will pick the second alternative. ! - 'D' should be NO_REGS when the frame pointer isn't live. ! The idea is to *make* it live by restricting the register allocator ! to the first alternative. This isn't needed for correctness ! but it produces better code for small functions: it's more ! efficient to save and restore er6 in the prologue & epilogue ! than to do it in a define_split. */ enum reg_class h8300_reg_class_from_letter (int c) *************** h8300_reg_class_from_letter (int c) *** 484,491 **** return DESTINATION_REGS; case 'D': ! /* The meaning of a constraint shouldn't change dynamically, so ! we can't make this NO_REGS. */ return GENERAL_REGS; case 'f': --- 480,487 ---- return DESTINATION_REGS; case 'D': ! if (!regs_ever_live[HFP_REG]) ! return NO_REGS; return GENERAL_REGS; case 'f': *************** h8300_swap_into_er6 (rtx addr) *** 3051,3060 **** { push (HARD_FRAME_POINTER_REGNUM); emit_move_insn (hard_frame_pointer_rtx, addr); - if (REGNO (addr) == SP_REG) - emit_move_insn (hard_frame_pointer_rtx, - plus_constant (hard_frame_pointer_rtx, - GET_MODE_SIZE (word_mode))); } /* Move the current value of er6 into ADDR and pop its old value --- 3047,3052 ---- *************** h8300_swap_into_er6 (rtx addr) *** 3063,3070 **** void h8300_swap_out_of_er6 (rtx addr) { ! if (REGNO (addr) != SP_REG) ! emit_move_insn (addr, hard_frame_pointer_rtx); pop (HARD_FRAME_POINTER_REGNUM); } --- 3055,3061 ---- void h8300_swap_out_of_er6 (rtx addr) { ! emit_move_insn (addr, hard_frame_pointer_rtx); pop (HARD_FRAME_POINTER_REGNUM); } Index: config/h8300/h8300.md =================================================================== RCS file: /cvs/gcc/gcc/gcc/config/h8300/h8300.md,v retrieving revision 1.286 diff -c -p -F^\([(a-zA-Z0-9_]\|#define\) -r1.286 h8300.md *** config/h8300/h8300.md 8 Jul 2004 03:40:33 -0000 1.286 --- config/h8300/h8300.md 9 Jul 2004 09:34:25 -0000 *************** (define_insn "movmd_internal_normal" *** 574,580 **** (mem:BLK (match_operand:HI 4 "register_operand" "1,1"))) (unspec [(match_operand:HI 5 "register_operand" "2,2") (match_operand:HI 6 "const_int_operand" "n,n")] UNSPEC_MOVMD) ! (clobber (match_operand:HI 0 "register_operand" "=d,??D")) (clobber (match_operand:HI 1 "register_operand" "=f,f")) (set (match_operand:HI 2 "register_operand" "=c,c") (const_int 0))] --- 574,580 ---- (mem:BLK (match_operand:HI 4 "register_operand" "1,1"))) (unspec [(match_operand:HI 5 "register_operand" "2,2") (match_operand:HI 6 "const_int_operand" "n,n")] UNSPEC_MOVMD) ! (clobber (match_operand:HI 0 "register_operand" "=d,!D")) (clobber (match_operand:HI 1 "register_operand" "=f,f")) (set (match_operand:HI 2 "register_operand" "=c,c") (const_int 0))] *************** (define_insn "movmd_internal" *** 591,597 **** (mem:BLK (match_operand:SI 4 "register_operand" "1,1"))) (unspec [(match_operand:HI 5 "register_operand" "2,2") (match_operand:HI 6 "const_int_operand" "n,n")] UNSPEC_MOVMD) ! (clobber (match_operand:SI 0 "register_operand" "=d,??D")) (clobber (match_operand:SI 1 "register_operand" "=f,f")) (set (match_operand:HI 2 "register_operand" "=c,c") (const_int 0))] --- 591,597 ---- (mem:BLK (match_operand:SI 4 "register_operand" "1,1"))) (unspec [(match_operand:HI 5 "register_operand" "2,2") (match_operand:HI 6 "const_int_operand" "n,n")] UNSPEC_MOVMD) ! (clobber (match_operand:SI 0 "register_operand" "=d,!D")) (clobber (match_operand:SI 1 "register_operand" "=f,f")) (set (match_operand:HI 2 "register_operand" "=c,c") (const_int 0))] *************** (define_insn "stpcpy_internal_normal" *** 702,708 **** [(set (mem:BLK (match_operand:HI 3 "register_operand" "0,r")) (unspec:BLK [(mem:BLK (match_operand:HI 4 "register_operand" "1,1"))] UNSPEC_STPCPY)) ! (clobber (match_operand:HI 0 "register_operand" "=d,??D")) (clobber (match_operand:HI 1 "register_operand" "=f,f")) (clobber (match_operand:HI 2 "register_operand" "=c,c"))] "TARGET_H8300SX && TARGET_NORMAL_MODE" --- 702,708 ---- [(set (mem:BLK (match_operand:HI 3 "register_operand" "0,r")) (unspec:BLK [(mem:BLK (match_operand:HI 4 "register_operand" "1,1"))] UNSPEC_STPCPY)) ! (clobber (match_operand:HI 0 "register_operand" "=d,!D")) (clobber (match_operand:HI 1 "register_operand" "=f,f")) (clobber (match_operand:HI 2 "register_operand" "=c,c"))] "TARGET_H8300SX && TARGET_NORMAL_MODE" *************** (define_insn "stpcpy_internal" *** 716,722 **** [(set (mem:BLK (match_operand:SI 3 "register_operand" "0,r")) (unspec:BLK [(mem:BLK (match_operand:SI 4 "register_operand" "1,1"))] UNSPEC_STPCPY)) ! (clobber (match_operand:SI 0 "register_operand" "=d,??D")) (clobber (match_operand:SI 1 "register_operand" "=f,f")) (clobber (match_operand:SI 2 "register_operand" "=c,c"))] "TARGET_H8300SX && !TARGET_NORMAL_MODE" --- 716,722 ---- [(set (mem:BLK (match_operand:SI 3 "register_operand" "0,r")) (unspec:BLK [(mem:BLK (match_operand:SI 4 "register_operand" "1,1"))] UNSPEC_STPCPY)) ! (clobber (match_operand:SI 0 "register_operand" "=d,!D")) (clobber (match_operand:SI 1 "register_operand" "=f,f")) (clobber (match_operand:SI 2 "register_operand" "=c,c"))] "TARGET_H8300SX && !TARGET_NORMAL_MODE"