From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 2039 invoked by alias); 24 Jan 2003 03:44:45 -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 2032 invoked from network); 24 Jan 2003 03:44:44 -0000 Received: from unknown (HELO faui11.informatik.uni-erlangen.de) (131.188.31.2) by 172.16.49.205 with SMTP; 24 Jan 2003 03:44:44 -0000 Received: (from weigand@localhost) by faui11.informatik.uni-erlangen.de (8.9.1/8.1.4-FAU) id EAA05156 for gcc-patches@gcc.gnu.org; Fri, 24 Jan 2003 04:44:43 +0100 (MET) From: Ulrich Weigand Message-Id: <200301240344.EAA05156@faui11.informatik.uni-erlangen.de> Subject: [PATCH] Fix find_reloads_address bug, take 2 To: gcc-patches@gcc.gnu.org Date: Fri, 24 Jan 2003 03:44:00 -0000 In-Reply-To: from "weigand" at Jan 23, 2003 12:17:48 AM MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7bit X-SW-Source: 2003-01/txt/msg01735.txt.bz2 I wrote originally: >Secondly, if the condition hits, the code in question re-forms >the address from > (plus (plus frame-pointer index) constant) >to > (plus (plus frame-pointer constant) index) >which is non-canonical RTL. That would not be a problem if >-as intended- the (plus frame-pointer constant) term got >reloaded into a register. However, find_reloads is called >multiple times, and only after the last call will actual >reloads be made. Unfortunatly, this code segment will >*modify* the passed-in address RTX in-place on *every* >call of find_reloads. This means after the return from >the first find_reloads call out of calculate_needs_all_insns, >the main insn stream will contain this piece of invalid RTL. While this is correct, it actually does not really matter. *If* the condition when to enter this block were correct, the constant term would be invalid for an address. This can only happen if that constant was in fact introduced by eliminate_regs_in_insn just before the call to find_reloads. In that case, it does not matter that the address is modified in place, as calculate_needs_all_insns will throw away the insn body anway to get rid to the changes done by eliminate_regs. The reason why I am seeing the ICE on s390 is that this block is entered -due to the incorrect condition- even for addresses where the constant term is valid; such addresses might have been in the insn stream even before register elimination, and thus calculate_needs_all_insns will not undo the modification ... Thus, the only bug that needs to be fixed is in fact the incorrect condition; the rest should be fine as is. The following patch implements this, and also fixes a stupid bug in the original patch: the lines ! && ! maybe_memory_address_p (mode, ad, &XEXP (XEXP (ad, 0), 0))) and ! && ! maybe_memory_address_p (mode, ad, &XEXP (XEXP (ad, 0), 1))) need to be swapped, of course. Also, I've included a test case that shows the ICE on s390. Bootstrapped/regtested on s390-ibm-linux and s390x-ibm-linux, on gcc 3.3 branch and mainline. OK to apply? Should this be considered for 3.2 as well? ChangeLog: gcc/ * reload.c (maybe_memory_address_p): New function. (find_reloads_address): Use it instead of memory_address_p. gcc/testsuite/ * gcc.dg/20030123-1.c: New test. Index: gcc/reload.c =================================================================== RCS file: /cvs/gcc/gcc/gcc/reload.c,v retrieving revision 1.178.2.4.2.4 diff -c -p -r1.178.2.4.2.4 reload.c *** gcc/reload.c 24 Oct 2002 08:59:49 -0000 1.178.2.4.2.4 --- gcc/reload.c 23 Jan 2003 20:21:16 -0000 *************** static int alternative_allows_memconst P *** 257,262 **** --- 257,263 ---- static rtx find_reloads_toplev PARAMS ((rtx, int, enum reload_type, int, int, rtx, int *)); static rtx make_memloc PARAMS ((rtx, int)); + static int maybe_memory_address_p PARAMS ((enum machine_mode, rtx, rtx *)); static int find_reloads_address PARAMS ((enum machine_mode, rtx *, rtx, rtx *, int, enum reload_type, int, rtx)); static rtx subst_reg_equivs PARAMS ((rtx, rtx)); *************** make_memloc (ad, regno) *** 4545,4550 **** --- 4546,4572 ---- return tem; } + /* Returns true if AD could be turned into a valid memory reference + to mode MODE by reloading the part pointed to by PART into a + register. */ + + static int + maybe_memory_address_p (mode, ad, part) + enum machine_mode mode; + rtx ad; + rtx *part; + { + int retv; + rtx tem = *part; + rtx reg = gen_rtx_REG (GET_MODE (tem), max_reg_num ()); + + *part = reg; + retv = memory_address_p (mode, ad); + *part = tem; + + return retv; + } + /* Record all reloads needed for handling memory address AD which appears in *LOC in a memory reference to mode MODE which itself is found in location *MEMREFLOC. *************** find_reloads_address (mode, memrefloc, a *** 4844,4850 **** || XEXP (XEXP (ad, 0), 0) == arg_pointer_rtx #endif || XEXP (XEXP (ad, 0), 0) == stack_pointer_rtx) ! && ! memory_address_p (mode, ad)) { *loc = ad = gen_rtx_PLUS (GET_MODE (ad), plus_constant (XEXP (XEXP (ad, 0), 0), --- 4866,4872 ---- || XEXP (XEXP (ad, 0), 0) == arg_pointer_rtx #endif || XEXP (XEXP (ad, 0), 0) == stack_pointer_rtx) ! && ! maybe_memory_address_p (mode, ad, &XEXP (XEXP (ad, 0), 1))) { *loc = ad = gen_rtx_PLUS (GET_MODE (ad), plus_constant (XEXP (XEXP (ad, 0), 0), *************** find_reloads_address (mode, memrefloc, a *** 4869,4875 **** || XEXP (XEXP (ad, 0), 1) == arg_pointer_rtx #endif || XEXP (XEXP (ad, 0), 1) == stack_pointer_rtx) ! && ! memory_address_p (mode, ad)) { *loc = ad = gen_rtx_PLUS (GET_MODE (ad), XEXP (XEXP (ad, 0), 0), --- 4891,4897 ---- || XEXP (XEXP (ad, 0), 1) == arg_pointer_rtx #endif || XEXP (XEXP (ad, 0), 1) == stack_pointer_rtx) ! && ! maybe_memory_address_p (mode, ad, &XEXP (XEXP (ad, 0), 0))) { *loc = ad = gen_rtx_PLUS (GET_MODE (ad), XEXP (XEXP (ad, 0), 0), *** /dev/null Thu Sep 19 11:33:10 2002 --- gcc/testsuite/gcc.dg/20030123-1.c Thu Jan 23 21:27:33 2003 *************** *** 0 **** --- 1,17 ---- + /* This used to ICE due to a reload bug on s390*. */ + + /* { dg-do compile { target s390*-*-* } } */ + /* { dg-options "-O2" } */ + + void func (char *p); + + void test (void) + { + char *p = alloca (4096); + long idx; + + asm ("" : "=r" (idx) : : "1", "2", "3", "4", "5", "6", "7", "8", "9", "10", "11", "12"); + + func (p + idx + 1); + } + -- Dr. Ulrich Weigand weigand@informatik.uni-erlangen.de