Triggered by this change: 2004-01-15 Zack Weinberg <zack@codesourcery.com> * config/ia64/ia64.md (*movti_internal): C output template extracted to ia64.c. (*movti_internal_reg): Delete. (reload_inti, reload_outti): Use the correct mode on operand 2 in the first place, don't fix it up in the output template. (movtf, reload_ointf, reload_outtf): New expanders. (*movtf_internal): New define_insn_and_split. * config/ia64/ia64.c (ia64_split_timode): Rename to ia64_split_tmode; make static; do not hand TFmode CONST_DOUBLEs to split_double. (ia64_split_tmode_move): New function, body mostly pulled from ia64.md:*movti_internal. (ia64_function_arg_words): New function, extracted common logic from ia64_function_arg et seq. (ia64_function_arg_offset): Likewise. Handle correctly the case of a scalar quantity 16 bytes wide with only 8-byte alignment. (ia64_function_arg, ia64_function_arg_partial_nregs) (ia64_function_arg_advance): Use ia64_function_arg_words and ia64_function_arg_offset. (ia64_function_value): TCmode does not go in float regs. (ia64_secondary_reload_class): Also handle TFmode. * config/ia64/ia64-protos.h: Remove prototype for ia64_split_timode; add prototype for ia64_split_tmode_move. stage1/xgcc -Bstage1/ -B/usr/local/ia64-suse-linux/bin/ -c -g -O2 -gnatpg -gnata -I- -I. -Iada -I../../gcc/ada ../../gcc/ada/a-elchha.adb -o ada/a-elchha.o +===========================GNAT BUG DETECTED==============================+ | 3.5.0 20040117 (experimental) (ia64-suse-linux-gnu) GCC error: | | in push_secondary_reload, at reload.c:427 | | Error detected at a-elchha.adb:104:8 | (gdb) r Starting program: /tmp/cvs/gcc-test-2004011604/Build/gcc/stage1/gnat1 -I- -I. -Iada -I../../gcc/ada -quiet -dumpbase a-elchha.adb -g -gnatpg -gnata -O2 -gnatO ada/a-elchha.o ../../gcc/ada/a-elchha.adb Breakpoint 1, fancy_abort (file=0x40000000013999a0 "../../gcc/reload.c", line=427, function=0x40000000013999b8 "push_secondary_reload") at ../../gcc/diagnostic.c:584 584 internal_error ("in %s, at %s:%d", function, trim_filename (file), line); (gdb) up #1 0x4000000000e06a70 in push_secondary_reload (in_p=1, x=0x2000000000582da0, opnum=1, optional=0, reload_class=GR_REGS, reload_mode=TImode, type=RELOAD_FOR_INPUT_ADDRESS, picode=0x60000fffffff6ed8) at ../../gcc/reload.c:427 427 abort (); (gdb) l 422 Allow this when a reload_in/out pattern is being used. I.e. assume 423 that the generated code handles this case. */ 424 425 if (in_p && class == reload_class && icode == CODE_FOR_nothing 426 && t_icode == CODE_FOR_nothing) 427 abort (); 428 429 /* If we need a tertiary reload, see if we have one we can reuse or else 430 make a new one. */ 431 (gdb) p class $1 = GR_REGS (gdb) p x $2 = 0x2000000000582da0 (gdb) pr (mem:TI (plus:DI (reg/f:DI 12 r12) (const_int 736 [0x2e0])) [17 buff+0 S16 A128])
Subject: Re: [3.4/3.5 regression] [ia64] Ada bootstrap failure I cannot debug Ada problems on ia64. zw
Subject: Re: [3.4/3.5 regression] [ia64] Ada bootstrap failure > I cannot debug Ada problems on ia64. Why ? A regular gdb understanding C can be used to debug Ada. Alternatively you can use http://libre.act-europe.fr/GDB if an Ada-aware gdb is really needed (for low level debugging of gdb, it usually isn't) Let me know if you need to help (e.g. don't hesitate to send me private emails) to get you started. Arno
Subject: Re: [3.4/3.5 regression] [ia64] Ada bootstrap failure I can't even *build* the ada compiler on ia64. zw
Subject: Re: [3.4/3.5 regression] [ia64] Ada bootstrap failure > I can't even *build* the ada compiler on ia64. Well yes, I guess this is what this PR is about, no ? :-) You should still be able to debug the stage1/gnat1 that is failing during the bootstrap. Or ask Andreas about his set up if you can't get to this point (you didn't give a clue as to why you can't build, so we're really playing guessing games here). Arno
Subject: Re: [3.4/3.5 regression] [ia64] Ada bootstrap failure The only ia64 platform I have convenient access to is ia64-hpux11.x (two point releases, which are not noticeably different). I do not have any Ada compiler for this target, therefore I cannot get as far as building stage1/gnat1. For all I know it isn't even supported. I really need someone else to try to debug this. zw
Subject: Re: [3.4/3.5 regression] [ia64] Ada bootstrap failure > The only ia64 platform I have convenient access to is ia64-hpux11.x > (two point releases, which are not noticeably different). I do not > have any Ada compiler for this target, therefore I cannot get as far > as building stage1/gnat1. For all I know it isn't even supported. You should be able to build a ia64-hpux Ada compiler (with no tasking, but that doesn't matter for bootstrap issues). Of course I have no idea whether the problem will show on this config. Arno
Subject: Re: [3.4/3.5 regression] [ia64] Ada bootstrap failure How do I build an ia64-hpux Ada compiler if I don't have one to begin with? zw
Subject: Re: [3.4/3.5 regression] [ia64] Ada bootstrap failure > How do I build an ia64-hpux Ada compiler if I don't have one to begin > with? By building a cross compiler. Arno
Subject: Re: [3.4/3.5 regression] [ia64] Ada bootstrap failure I think it'll be easier for everyone involved if someone who already sees this problem will debug it a bit more. zw
Subject: Re: [3.4/3.5 regression] [ia64] Ada bootstrap failure > I think it'll be easier for everyone involved if someone who already > sees this problem will debug it a bit more. Yes, I agree this makes more sense for the time being. Arno
Created attachment 5535 [details] Debugging dumps
Created attachment 5543 [details] C testcase for the problem The bug can also be reproduced with this c testcase: ./cc1 tc.i ... init_iconv_desc cppcharset.c: In function `init_iconv_desc': cppcharset.c:704: internal compiler error: in push_secondary_reload, at reload.c:427
Reduces to: struct cset_converter { void * func; char cd; }; extern void cpp_error (void *, int, char *,...); static struct cset_converter init_iconv_desc (void *to) { struct cset_converter ret; if(to) return ret; cpp_error (to, 0x03, "foo"); return ret; }
Subject: Re: [3.4/3.5 regression] [ia64] ICE in push_secondary_reload See the MIME attachment. I see Zack has left yet another IA-64 turd for me to clean up. Sigh. The main problem with Zack's patch is that he replaced a null predicate in the reload_{in,out}* patterns with a memory_operand predicate. This doesn't work. push_secondary_reloads tries to verify that the operands are OK for the secondary reload pattern. However, for a MEM, this means that we are passing an operand to the predicate that may appear to be invalid, but is valid because it has already been reloaded. memory_operand doesn't know this, and returns false. The IA-64 port deliberately used a null predicate to make this work. I see the Alpha port defines a special any_memory_operand predicate for this case. I blatantly copied it from the Alpha port. By the way, the Alpha PREDICATE_CODES seems to be broken, because it says any_memory_operand only accepts MEM, but it also accepts SUBREG. This is enough to make the small C example work. However, I think there are other problems. Zack "fixed" the secondary reload patterns to use DImode instead of TImode for operand2. Anyone who knows how these patterns work knows that you have to allocate 2 registers just in case the scratch operand overlaps one of the other operands. Practically all ports define the patterns this way. If this case happens, we can hit the abort in ia64_split_tmode_move. The original code was trying to avoid this problem. It was doing it rather inexpertly, and perhaps incorretly, but the general idea here was right. Not surprisingly, this problem turns up during an Ada build. On multiple files. I haven't written a patch to fix this yet. This will take a bit of thought, because we need to handle overlap both with the input operand and the output operand. We might actually need to use OImode to make this safe. Or maybe we can fix this by changing ia64_split_tmode to decide based on overlaps whether it should use the scratch register for out[0] or out[1]. The example I looked at had an insn [(set (reg:TI r14) (mem:TI (reg:DI r15))) (clobber (reg:DI r14))] which resulted in (set (reg:DI r15) (plus:DI (reg:DI r14) (const_int 8))) (set (reg:DI r14) (mem:DI (reg:DI r15))) (set (reg:DI r15) (mem:DI (reg:DI r14))) and ia64_split_tmode_move aborts because this fails no matter which order we try to use for the last two instructions. If we were smarter about the input addresses, we could have copied r14 to r15, put the sum in r14, and then we would have (set (reg:DI r15) (reg:DI r14)) (set (reg:DI r14) (plus:DI (reg:DI r14) (const_int 8))) (set (reg:DI r14) (mem:DI (reg:DI r14))) (set (reg:DI r15) (mem:DI (reg:DI r15))) which does work. This is one instruction longer, but may be rare enough that it isn't a problem. I haven't looked at the rest of the patch in detail yet, so there could be other problems too. 2004-01-21 James E Wilson <wilson@specifixinc.com> * config/ia64/ia64.c (any_memory_operand): New. * config/ia64/ia64.h (PREDICATE_CODES): Add any_memory_operand. * config/ia64/ia64.md (reload_inti, reload_outti, reload_intf, reload_outtf): Use any_memory_operand. Index: ia64.c =================================================================== RCS file: /cvs/gcc/gcc/gcc/config/ia64/ia64.c,v retrieving revision 1.265 diff -p -r1.265 ia64.c *** ia64.c 16 Jan 2004 01:27:37 -0000 1.265 --- ia64.c 22 Jan 2004 00:28:08 -0000 *************** basereg_operand (rtx op, enum machine_mo *** 961,966 **** --- 961,980 ---- return (register_operand (op, mode) && REG_POINTER ((GET_CODE (op) == SUBREG) ? SUBREG_REG (op) : op)); } + + /* Return 1 if OP is any memory location. During reload a pseudo matches. */ + + int + any_memory_operand (rtx op, enum machine_mode mode ATTRIBUTE_UNUSED) + { + return (GET_CODE (op) == MEM + || (GET_CODE (op) == SUBREG && GET_CODE (SUBREG_REG (op)) == REG) + || (reload_in_progress && GET_CODE (op) == REG + && REGNO (op) >= FIRST_PSEUDO_REGISTER) + || (reload_in_progress && GET_CODE (op) == SUBREG + && GET_CODE (SUBREG_REG (op)) == REG + && REGNO (SUBREG_REG (op)) >= FIRST_PSEUDO_REGISTER)); + } typedef enum { Index: ia64.h =================================================================== RCS file: /cvs/gcc/gcc/gcc/config/ia64/ia64.h,v retrieving revision 1.163 diff -p -r1.163 ia64.h *** ia64.h 6 Dec 2003 05:40:14 -0000 1.163 --- ia64.h 22 Jan 2004 00:28:11 -0000 *************** do { \ *** 2249,2255 **** { "general_xfmode_operand", {SUBREG, REG, CONST_DOUBLE, MEM}}, \ { "destination_xfmode_operand", {SUBREG, REG, MEM}}, \ { "xfreg_or_fp01_operand", {REG, CONST_DOUBLE}}, \ ! { "basereg_operand", {SUBREG, REG}}, /* An alias for a machine mode name. This is the machine mode that elements of a jump-table should have. */ --- 2249,2256 ---- { "general_xfmode_operand", {SUBREG, REG, CONST_DOUBLE, MEM}}, \ { "destination_xfmode_operand", {SUBREG, REG, MEM}}, \ { "xfreg_or_fp01_operand", {REG, CONST_DOUBLE}}, \ ! { "basereg_operand", {SUBREG, REG}}, \ ! { "any_memory_operand", {SUBREG, MEM}}, /* An alias for a machine mode name. This is the machine mode that elements of a jump-table should have. */ Index: ia64.md =================================================================== RCS file: /cvs/gcc/gcc/gcc/config/ia64/ia64.md,v retrieving revision 1.119 diff -p -r1.119 ia64.md *** ia64.md 16 Jan 2004 01:27:38 -0000 1.119 --- ia64.md 22 Jan 2004 00:28:17 -0000 *************** *** 614,626 **** (define_expand "reload_inti" [(parallel [(set (match_operand:TI 0 "register_operand" "=r") ! (match_operand:TI 1 "memory_operand" "m")) (clobber (match_operand:DI 2 "register_operand" "=&r"))])] "" "") (define_expand "reload_outti" ! [(parallel [(set (match_operand:TI 0 "memory_operand" "=m") (match_operand:TI 1 "register_operand" "r")) (clobber (match_operand:DI 2 "register_operand" "=&r"))])] "" --- 614,626 ---- (define_expand "reload_inti" [(parallel [(set (match_operand:TI 0 "register_operand" "=r") ! (match_operand:TI 1 "any_memory_operand" "m")) (clobber (match_operand:DI 2 "register_operand" "=&r"))])] "" "") (define_expand "reload_outti" ! [(parallel [(set (match_operand:TI 0 "any_memory_operand" "=m") (match_operand:TI 1 "register_operand" "r")) (clobber (match_operand:DI 2 "register_operand" "=&r"))])] "" *************** *** 796,808 **** (define_expand "reload_intf" [(parallel [(set (match_operand:TF 0 "register_operand" "=r") ! (match_operand:TF 1 "memory_operand" "m")) (clobber (match_operand:DI 2 "register_operand" "=&r"))])] "" "") (define_expand "reload_outtf" ! [(parallel [(set (match_operand:TF 0 "memory_operand" "=m") (match_operand:TF 1 "register_operand" "r")) (clobber (match_operand:DI 2 "register_operand" "=&r"))])] "" --- 796,808 ---- (define_expand "reload_intf" [(parallel [(set (match_operand:TF 0 "register_operand" "=r") ! (match_operand:TF 1 "any_memory_operand" "m")) (clobber (match_operand:DI 2 "register_operand" "=&r"))])] "" "") (define_expand "reload_outtf" ! [(parallel [(set (match_operand:TF 0 "any_memory_operand" "=m") (match_operand:TF 1 "register_operand" "r")) (clobber (match_operand:DI 2 "register_operand" "=&r"))])] ""
Subject: Re: [3.4/3.5 regression] [ia64] ICE in push_secondary_reload "wilson at specifixinc dot com" <gcc-bugzilla@gcc.gnu.org> writes: > I see Zack has left yet another IA-64 turd for me to clean up. Sigh. ... excuse me for having bigger problems on my plate right now. Not to mention that none of this is reproducible on any target I can conveniently test on. > The main problem with Zack's patch is that he replaced a null > predicate in the reload_{in,out}* patterns with a memory_operand > predicate. This doesn't work. push_secondary_reloads tries to > verify that the operands are OK for the secondary reload pattern. > However, for a MEM, this means that we are passing an operand to the > predicate that may appear to be invalid, but is valid because it has > already been reloaded. I ... see. > However, I think there are other problems. Zack "fixed" the > secondary reload patterns to use DImode instead of TImode for > operand2. Anyone who knows how these patterns work knows that you > have to allocate 2 registers just in case the scratch operand > overlaps one of the other operands. Practically all ports define > the patterns this way. This is insane. Clearly the bug is that overlapping operands can occur in the first place. zw
Subject: Re: [3.4/3.5 regression] [ia64] ICE in push_secondary_reload zack at codesourcery dot com wrote: > ... excuse me for having bigger problems on my plate right now. Not > to mention that none of this is reproducible on any target I can > conveniently test on. The simple fact is that you have broken the IA-64 compiler a number of times, and you have not always been responsive to fixing the problems you have caused. This is very annoying. There are many people using the IA-64 port, and you inconvience them everytime you break it. I have important work to do also, but instead I am forced to do your work for you, and I am not happy about this. If you aren't going to try to do IA-64 work properly, then you could at least post patches for review instead of just blindly checking them in and waiting to see what breaks. Or you could voluntarily revert patches when it is proven that they don't work. This is much more friendly to the rest of us, particularly those of us using the IA-64 gcc port for real work. debian linux is and always has been free. If you have an IA-64 machine, it should be easy to install debian on it. >>However, for a MEM, this means that we are passing an operand to the >>predicate that may appear to be invalid, but is valid because it has >>already been reloaded. Maybe the issue is that it hasn't been reloaded yet? In any case, we are in the middle of reload, so we are guaranteed that a MEM is going to be OK whether it looks OK or not. > This is insane. Clearly the bug is that overlapping operands can > occur in the first place. It has always worked this way. I know this is lame, but one should never underestimate the difficulty of trying to change how reload works. It is much easier to change all md file than to change reload. I think there is an important reason why it works this way, but it probably isn't possible to figure it out without spending an unreasonable amount of time messing with the code.
Subject: Re: [3.4/3.5 regression] [ia64] ICE in push_secondary_reload "wilson at specifixinc dot com" <gcc-bugzilla@gcc.gnu.org> writes: > The simple fact is that you have broken the IA-64 compiler a number of > times, and you have not always been responsive to fixing the problems > you have caused. This is very annoying. There are many people using > the IA-64 port, and you inconvience them everytime you break it. > > I have important work to do also, but instead I am forced to do your > work for you, and I am not happy about this. > > If you aren't going to try to do IA-64 work properly, then you could at > least post patches for review instead of just blindly checking them in > and waiting to see what breaks. Or you could voluntarily revert patches > when it is proven that they don't work. This is much more friendly to > the rest of us, particularly those of us using the IA-64 gcc port for > real work. The fact of the matter is that I *have* always tested these changes on an ia64 machine. It runs HP/UX, which (a) is big-endian, and (b) does not (to my knowledge) have an Ada port yet. Therefore testing on ia64-linux is, unfortunately, going to show problems that my testing cannot. It is not feasible for me to change the operating system on the machine. The implication of what you are saying above is that you don't think anyone without access to an ia64-linux box should be allowed to touch the ia64 back end, which I think is bad policy. It is certainly the case that I don't know as much about coding machine descriptions as some people do, and if you would like, I will stop checking backend patches in immediately, to give others a chance to find problems - but my experience is that everyone ignores me when I ask for a second opinion on my patches, so I don't expect this to help much. I *have* managed to reproduce the original bootstrap failure with an i686-linux->ia64-hpux Ada cross compiler, for the record. > It has always worked this way. I know this is lame, but one should > never underestimate the difficulty of trying to change how reload works. > It is much easier to change all md file than to change reload. I > think there is an important reason why it works this way, but it > probably isn't possible to figure it out without spending an > unreasonable amount of time messing with the code. RTH and I talked this over on IRC and came up with a clever plan, which I intend to implement tonight and post for review, assuming it works, tomorrow morning. zw
Several messages regarding this bug were sent to PR 13772 by mistake.
Subject: Re: PR 13722 candidate fix [Note corrected bug number. 13722, not 13772.] Richard Henderson <rth@redhat.com> writes: > On Thu, Jan 22, 2004 at 02:07:12PM +0100, Andreas Schwab wrote: >> (mem/s:TI (post_modify:DI (reg/f:DI 36 loc4 [400]) >> (plus:DI (reg/f:DI 36 loc4 [400]) >> (const_int 8 [0x8]))) [4 opt__R47s+0 S16 A128]) > > Yep, looking at the patch, I was about to remind Zack that > (1) post-modify can take registers as well as constants, and > (2) you have to check for overflowing the 9-bit post-modify > constant. That shouldn't be hard... I suppose the thing to do is emit adddi3 instructions to fix up, if either condition is true. > Hum, and, actually, that post-modify seems very improbable. > addr += addr? There's sure to be a typo somewhere. Okay, what is the form of a post_modify expression supposed to be? This is coming from switch (base) { ... case POST_MODIFY: /* Extract and adjust the modification. */ offset = XEXP (base, 1); base = XEXP (base, 0); out[0] = change_address (in, DImode, gen_rtx_POST_INC (Pmode, base)); out[1] = change_address (in, DImode, gen_rtx_POST_MODIFY (Pmode, base, plus_constant (offset, -8))); break; and it sure *looks* consistent with what it says in the manual about the post_modify RTL, which gives (mem:SF (post_modify:SI (reg:SI 42) (plus (reg:SI 42) (reg:SI 48)))) as an exemplar. (Still looking for a clue about the differences among change_address, adjust_address, offset_address, etc.) zw
Subject: Re: PR 13722 candidate fix On Thu, Jan 22, 2004 at 12:20:10PM -0800, Zack Weinberg wrote: > Okay, what is the form of a post_modify expression supposed to be? My eyes weren't awake, sorry. It's addr = addr + c, not +=. > (Still looking for a clue about the differences among change_address, > adjust_address, offset_address, etc.) change_address can make arbitrary changes to a memory. About all we assume is that we're pointing to the same object. But we lose track of offset within the object, and all alignment not inferrable from the mode of the memory. adjust_address modifies a memory by adding a HWI to the address. We get to keep a lot of data about alignment, offset within an object. offset_address is similar, except that it takes an rtx. adjust_automodify_address is like adjust_address in that it computes new MEM_ATTRS as if adding a HWI to a base address, except that it allows you to give a completely new rtx for the address at the same time. The assumption being that the new rtx is some automodify, but that's not actually checked. This last is the one you want. r~
Subject: Re: [3.4/3.5 regression] [ia64] ICE in push_secondary_reload zack at codesourcery dot com wrote: > The implication of what you are saying above is that you don't think > anyone without access to an ia64-linux box should be allowed to touch > the ia64 back end, which I think is bad policy. This is one way to solve the problem, but not the only way. I mention it because I think it is the most expedient. Another solution would be for you to build an ia64-hpux Ada compiler. Another solution is for you to voluntarily revert patches when it is discovered that they don't work. Another solution is for you to ask for help testing patches before checking them in. > to find problems - but my experience is that everyone ignores me when > I ask for a second opinion on my patches, so I don't expect this to > help much. I don't ignore IA-64 backend patches, though I am sometimes a week or so behind on reading gcc-patches. I wouldn't mind if you cc'ed me on IA-64 backend patches so that I would see them sooner.
Subject: Re: PR 13722 candidate fix Andreas Schwab <schwab@suse.de> writes: > Still doesn't fly. Revised patch follows. Under what circumstances should REG_INC notes be tagged onto the insns emitted here? It's currently done only for POST_MODIFYs, but this patch will generate POST_INC and POST_DEC as well. zw
Created attachment 5559 [details] pr13722.diff
Subject: Re: PR 13722 candidate fix On Thu, Jan 22, 2004 at 11:43:40PM -0800, Zack Weinberg wrote: > + out[0] = adjust_automodify_address > + (in, DImode, gen_rtx_POST_INC (Pmode, base), 0); > + > + if (GET_CODE (XEXP (offset, 1)) == REG) > + { > + rtx reg = XEXP (offset, 1); > + /* Tweak the index register, then emit the first move as > + a POST_INC and the second as the original POST_MODIFY. */ > + emit_insn (GEN_FCN (add_optab->handlers[GET_MODE (reg)].insn_code) > + (reg, reg, GEN_INT (-8))); You can't modify the POST_MODIFY operand. You need to modify the address register a third time, ie. ld8 x = [a], 8 ld8 y = [a], -8 add a = r, a or ld8 x = [a], 8 ld8 y = [a], r adds a = -8, a r~
Subject: Re: PR 13722 candidate fix Richard Henderson <rth@redhat.com> writes: > On Thu, Jan 22, 2004 at 11:43:40PM -0800, Zack Weinberg wrote: >> + out[0] = adjust_automodify_address >> + (in, DImode, gen_rtx_POST_INC (Pmode, base), 0); >> + >> + if (GET_CODE (XEXP (offset, 1)) == REG) >> + { >> + rtx reg = XEXP (offset, 1); >> + /* Tweak the index register, then emit the first move as >> + a POST_INC and the second as the original POST_MODIFY. */ >> + emit_insn (GEN_FCN (add_optab->handlers[GET_MODE (reg)].insn_code) >> + (reg, reg, GEN_INT (-8))); > > You can't modify the POST_MODIFY operand. > You need to modify the address register a third time, ie. Duh, because the index register value might be needed again. This isn't true for constant POST_MODIFY, though, right? (Gets me out of having to dig around in add_optab, though, yay.) zw
Subject: Re: PR 13722 candidate fix Re-revised patch. zw
Created attachment 5560 [details] pr13722.diff
Subject: Re: PR 13722 candidate fix "Zack Weinberg" <zack@codesourcery.com> writes: > Re-revised patch. Seems to miscompile the stage2 compiler. $ stage2/xgcc -Bstage2/ -B/usr/local/ia64-suse-linux/bin/ -c -g -O2 -gnatpg -gnata -I- -I. -Iada -I../../gcc/ada ../../gcc/ada/ada.ads -o ada/ada.o -v Reading specs from stage2/specs Configured with: ../configure --host=ia64-suse-linux --enable-shared --enable-threads --with-system-zlib Thread model: posix gcc version 3.5.0 20040123 (experimental) stage2/gnat1 -I- -I. -Iada -I../../gcc/ada -quiet -dumpbase ada.ads -g -gnatpg -gnata -O2 -gnatO ada/ada.o ../../gcc/ada/ada.ads -o /tmp/ccR0NvnR.s fatal error, run-time library not installed correctly cannot locate file system.ads compilation abandoned strace shows that gnat1 wants to stat() "" three times, whereas the stage1 compiler searches for "./system.ads", "ada/system.ads", "../../gcc/ada/system.ads" in that order. Looking at the contents of the empty strings shows that they actually point to "\0/system.ads", "\0\0\0/system.ads" and "\0\0\0\0\0\0\0\0\0\0\0\0\0/system.ads", ie. the characters before "/system.ads" are replaced by '\0'. Andreas.
Subject: Re: PR 13722 candidate fix Andreas Schwab <schwab@suse.de> writes: > "Zack Weinberg" <zack@codesourcery.com> writes: > >> Re-revised patch. > > Seems to miscompile the stage2 compiler. > > $ stage2/xgcc -Bstage2/ -B/usr/local/ia64-suse-linux/bin/ -c -g -O2 -gnatpg -gnata -I- -I. -Iada -I../../gcc/ada ../../gcc/ada/ada.ads -o ada/ada.o -v > Reading specs from stage2/specs > Configured with: ../configure --host=ia64-suse-linux --enable-shared --enable-threads --with-system-zlib > Thread model: posix > gcc version 3.5.0 20040123 (experimental) > stage2/gnat1 -I- -I. -Iada -I../../gcc/ada -quiet -dumpbase ada.ads -g -gnatpg -gnata -O2 -gnatO ada/ada.o ../../gcc/ada/ada.ads -o /tmp/ccR0NvnR.s > fatal error, run-time library not installed correctly > cannot locate file system.ads > compilation abandoned Could you repeat the failing command with the added option -gnatdl, and send me the output? zw
Subject: Re: PR 13722 candidate fix "Zack Weinberg" <zack@codesourcery.com> writes: > Andreas Schwab <schwab@suse.de> writes: > >> "Zack Weinberg" <zack@codesourcery.com> writes: >> >>> Re-revised patch. >> >> Seems to miscompile the stage2 compiler. >> >> $ stage2/xgcc -Bstage2/ -B/usr/local/ia64-suse-linux/bin/ -c -g -O2 -gnatpg -gnata -I- -I. -Iada -I../../gcc/ada ../../gcc/ada/ada.ads -o ada/ada.o -v >> Reading specs from stage2/specs >> Configured with: ../configure --host=ia64-suse-linux --enable-shared --enable-threads --with-system-zlib >> Thread model: posix >> gcc version 3.5.0 20040123 (experimental) >> stage2/gnat1 -I- -I. -Iada -I../../gcc/ada -quiet -dumpbase ada.ads -g -gnatpg -gnata -O2 -gnatO ada/ada.o ../../gcc/ada/ada.ads -o /tmp/ccR0NvnR.s >> fatal error, run-time library not installed correctly >> cannot locate file system.ads >> compilation abandoned > > Could you repeat the failing command with the added option -gnatdl, > and send me the output? Unfortunately, there is no additional output. Andreas.
Subject: Re: PR 13722 candidate fix Andreas Schwab <schwab@suse.de> writes: >> Could you repeat the failing command with the added option -gnatdl, >> and send me the output? > > Unfortunately, there is no additional output. I'm at a bit of a loss trying to find the miscompilation. It's going wrong somewhere either in Name_Find or Load_Source_File or their subroutines, and I don't know which. Could you try to run it under the debugger and find out where the broken code is? zw
Subject: Re: PR 13722 candidate fix On Thu, 2004-01-22 at 23:43, Zack Weinberg wrote: > Under what circumstances should REG_INC notes be tagged onto the insns > emitted here? It's currently done only for POST_MODIFYs, but this > patch will generate POST_INC and POST_DEC as well. I believe the answer is that we need it for all auto-inc addresses. The REG_INC note indicates that the instruction has another side-effect besides the sets and clobbers. Any routines that care about instruction side-effects aren't going to notice the auto-inc address unless the REG_INC note is there. The fact that we only do this after reload reduces the chances of a problem, but the rtlanal.c reg_set_p function needs it, and it is likely that there is already some code somewhere after reload that directly or indirectly calls reg_set_p. Thus the REG_INC notes should be added to be safe.
Subject: Re: PR 13722 candidate fix "Zack Weinberg" <zack@codesourcery.com> writes: > I'm at a bit of a loss trying to find the miscompilation. It's going > wrong somewhere either in Name_Find or Load_Source_File or their > subroutines, and I don't know which. Could you try to run it under > the debugger and find out where the broken code is? I tried, but unfortunately gdb (even the one from CVS) is unable to properly interpret the debug information that gcc is generating, making it pretty much impossible to track variables and their values. Andreas.
Subject: Re: PR 13722 candidate fix On Fri, 2004-01-23 at 00:47, Zack Weinberg wrote: > Re-revised patch. I started a bootstrap as soon as I saw the patch. It just failed. I will try taking a look at this, but I don't know if I will have enough time to do anything useful. I did take a look at the patch. You mention reload nightmares. There is a reload nightmare only if you fight how reload works. If we allocate an OImode scratch instead of a DImode scratch, then I think we will always get at least one safe scratch register. The instruction can use at most 4 registers, 2 for the register operand, 1 for the address of the memory operand, and optionally one for the auto-inc operand in the memory operand. In the worst case, where we have complete overlap between the 4 scratch registers and the 4 insn registers, it gets more complicated. If we are doing a load, then we can use the dest register that is loaded last as the scratch register. If we are doing a store, hmm, I guess in this case we need a workaround, but this is just one very specific case that needs a workaround and it is going to be very rare. Keep in mind that Itanium2 has 4 memory ports. It can do up to 2 loads and 2 stores per cycle. Using auto-inc addresses instead of a scratch reg eliminates ILP, and will hurt performance. The TImode stuff perhaps doesn't occur often enough to worry about, but I think this would seriously hurt ia64-hpux long double performance. I think we should consider whether this performance hit is acceptable. It might be acceptable to get the compiler working again, but at the very least I think it makes sense to add some ??? comments pointing out that we have an optimization problem. At least this doesn't affect ia64-linux long double performance. SUBREG is valid anywhere where a REG is valid. I see a number of places where you handle REG but not SUBREG. Also, I see one place where you assume a value is a REG without checking first. The address of a MEM could be a SUBREG. The operand to POST_MODIFY could be a SUBREG. The "register" operand could be a SUBREG. When you check for overlap between the "register" operand and a MEM, we need to check for SUBREGs. This problem and the missing REG_INC notes may be enough to explain why it doesn't work. I'll try adding simple checks for SUBREGs to see if this problem actually occurs during an Ada bootstrap.
Subject: Re: PR 13722 candidate fix Andreas Schwab <schwab@suse.de> writes: > "Zack Weinberg" <zack@codesourcery.com> writes: > >> I'm at a bit of a loss trying to find the miscompilation. It's going >> wrong somewhere either in Name_Find or Load_Source_File or their >> subroutines, and I don't know which. Could you try to run it under >> the debugger and find out where the broken code is? > > I tried, but unfortunately gdb (even the one from CVS) is unable to > properly interpret the debug information that gcc is generating, making it > pretty much impossible to track variables and their values. This unfortunately puts us into shot-in-the-dark territory. However, Jim pointed out one problem (lack of REG_INC notes in all cases) and I found another (probably more serious; fixup insns generated for unadjustible POST_MODIFY were being emitted in the wrong place). So, if you wouldn't mind trying the appended revision (changes only ia64.c)? zw
Created attachment 5566 [details] pr13772.3.diff
Subject: Re: PR 13722 candidate fix "Zack Weinberg" <zack@codesourcery.com> writes: > So, if you wouldn't mind trying the appended revision (changes only > ia64.c)? Does that mean that the changes in ia64.md from pr13722.diff should be backed out? Andreas.
Subject: Re: PR 13722 candidate fix On Fri, 2004-01-23 at 00:47, Zack Weinberg wrote: > Re-revised patch. On the REG/SUBREG issue, I was looking at your latest patch. In the full context of the previous patch, I see you already handle most of these issues by having a default switch case that aborts. The only real issue I see here is in the POST_MODIFY code where you have + if (GET_CODE (XEXP (offset, 1)) == REG) ... + else if (INTVAL (XEXP (offset, 1)) < -256 + 8) which assumes without checking that the offset is a CONST_INT if it isn't a REG. I put in an abort to see if this ever happens, but I doubt that it does. There is also a possible issue where you call reg_overlap_mentioned_p, and then assume that we have registers without checking. However, since we are already calling ia64_split_tmode, and that aborts for subregs, I think this is actually safe optimization wise. I think this can only fail if you have rtl checking enabled, and we have a subreg, in which we get the abort before calling ia64_split_tmode instead of inside ia64_split_tmode which isn't a serious problem. I see you have another modified patch. I can try this when my current build finishes.
Subject: Re: PR 13722 candidate fix On Fri, Jan 23, 2004 at 10:32:51PM +0100, Andreas Schwab wrote: > strace shows that gnat1 wants to stat() "" three times, whereas the stage1 > compiler searches for "./system.ads", "ada/system.ads", > "../../gcc/ada/system.ads" in that order. Looking at the contents of the > empty strings shows that they actually point to "\0/system.ads", > "\0\0\0/system.ads" and "\0\0\0\0\0\0\0\0\0\0\0\0\0/system.ads", ie. the > characters before "/system.ads" are replaced by '\0'. Could this be a miscompilation of __builtin_memcpy or the like? r~
Subject: Re: PR 13722 candidate fix Andreas Schwab <schwab@suse.de> writes: > "Zack Weinberg" <zack@codesourcery.com> writes: > >> So, if you wouldn't mind trying the appended revision (changes only >> ia64.c)? > > Does that mean that the changes in ia64.md from pr13722.diff should be > backed out? No, what I meant was, leave ia64.md alone and just re-patch ia64.c. zw
Subject: Re: PR 13722 candidate fix Jim Wilson <wilson@specifixinc.com> writes: > On Fri, 2004-01-23 at 00:47, Zack Weinberg wrote: >> Re-revised patch. > > On the REG/SUBREG issue, I was looking at your latest patch. In the > full context of the previous patch, I see you already handle most of > these issues by having a default switch case that aborts. Yes. I think there are good /a priori/ reasons to assume that we won't ever get SUBREGs here, so I think this is adequate. (A SUBREG in the operand itself could only be paradoxical, and shouldn't survive this far; a SUBREG in the MEM expression would either be paradoxical, and again shouldn't have survived this far, or would take it out of Pmode and therefore indicate a bug elsewhere.) > The only real issue I see here is in the POST_MODIFY code where you > have > + if (GET_CODE (XEXP (offset, 1)) == REG) > ... > + else if (INTVAL (XEXP (offset, 1)) < -256 + 8) > which assumes without checking that the offset is a CONST_INT if it > isn't a REG. I put in an abort to see if this ever happens, but I doubt > that it does. The theory here was that INTVAL would trigger an RTL checking abort if offset wasn't a CONST_INT, so there was no need for an explicit check. I now remember that RTL checking isn't on by default, so an explicit check would probably be wise. > I see you have another modified patch. I can try this when my current > build finishes. I would appreciate that. zw
Subject: Re: PR 13722 candidate fix Jim Wilson <wilson@specifixinc.com> writes: > You mention reload nightmares. There is a reload nightmare only if you > fight how reload works. If we allocate an OImode scratch instead of a > DImode scratch, then I think we will always get at least one safe > scratch register. [...] I am more comfortable coding it this way, and I think it will be easier to understand in six months even without benefit of lengthy commentary. Furthermore, I would like to eliminate OImode from the ia64 back end (currently it's only used for STACK_SAVEAREA_MODE, which should be handled differently) so I would prefer a solution that doesn't need it. [...] > Keep in mind that Itanium2 has 4 memory ports. It can do up to 2 loads > and 2 stores per cycle. Using auto-inc addresses instead of a scratch > reg eliminates ILP, and will hurt performance. The TImode stuff perhaps > doesn't occur often enough to worry about, but I think this would > seriously hurt ia64-hpux long double performance. I think we should > consider whether this performance hit is acceptable. It might be > acceptable to get the compiler working again, but at the very least I > think it makes sense to add some ??? comments pointing out that we have > an optimization problem. At least this doesn't affect ia64-linux long > double performance. This, I think, can be addressed with peephole2 patterns, which have the ability to allocate scratch registers without risk of overlap. A form like (define_peephole2 [(match_scratch:DI 3 "r") (set (match_operand:DI 0 "register_operand" "") (mem:DI (post_inc:DI (match_operand:DI 2 "register_operand" "")))) (set (match_operand:DI 1 "register_operand" "") (mem:DI (post_dec:DI (match_dup 2)))) (match_dup 3)] "!optimize_size" [(set (match_dup 3) (add:DI (match_dup 2) (const_int 8))) (set (match_dup 0) (mem:DI (match_dup 2))) (set (match_dup 1) (mem:DI (match_dup 3)))] "") will clean up the majority of these - note that it also gives us the ability to *not* use a scratch pointer when optimizing for size, and will just work in cases where there aren't scratch regs to be had. (I am not claiming that the above is exactly the way it should be written. Perhaps it ought to be done by matching the MEMs and changing them with adjust_automodify_address, or something like that.) zw
Subject: Re: PR 13722 candidate fix "Zack Weinberg" <zack@codesourcery.com> writes: > So, if you wouldn't mind trying the appended revision (changes only > ia64.c)? Unfortunately that didn't help, still the same problem with finding system.ads. Andreas.
Subject: Re: PR 13722 candidate fix Andreas Schwab <schwab@suse.de> writes: > "Zack Weinberg" <zack@codesourcery.com> writes: > >> So, if you wouldn't mind trying the appended revision (changes only >> ia64.c)? > > Unfortunately that didn't help, still the same problem with finding > system.ads. Ugh. I'm running out of ideas. Just to be sure this isn't an unrelated bug, could you revert ia64.c to version 1.264 and ia64.md to version 1.118, and see if it works again? (That is, backing out both these fixes and the original problematic change.) zw
Subject: Re: PR 13722 candidate fix On Fri, 2004-01-23 at 17:50, Zack Weinberg wrote: > This, I think, can be addressed with peephole2 patterns, which have > the ability to allocate scratch registers without risk of overlap. A > form like I forgot that you had mentioned peephole2 earlier. I am not very familiar with this code. I looked at the docs and looked at the code a bit, and I see that this does look like it will work nicely.
Subject: Re: PR 13722 candidate fix Jim Wilson <wilson@specifixinc.com> writes: > On Fri, 2004-01-23 at 17:50, Zack Weinberg wrote: >> This, I think, can be addressed with peephole2 patterns, which have >> the ability to allocate scratch registers without risk of overlap. A >> form like > > I forgot that you had mentioned peephole2 earlier. I am not very > familiar with this code. I looked at the docs and looked at the code a > bit, and I see that this does look like it will work nicely. Ok. I would prefer to implement that in a separate patch after we fix the bootstrap failure, though. zw
Subject: Re: PR 13722 candidate fix On Fri, 2004-01-23 at 19:34, Zack Weinberg wrote: > Ok. I would prefer to implement that in a separate patch after we fix > the bootstrap failure, though. Yes, that is fine. The important thing is that you do have a sensible plan, which I had forgotten about.
Subject: Re: PR 13722 candidate fix "Zack Weinberg" <zack@codesourcery.com> writes: > Just to be sure this isn't an unrelated bug, could you revert ia64.c > to version 1.264 and ia64.md to version 1.118, and see if it works > again? (That is, backing out both these fixes and the original > problematic change.) The regression tester is already running here with the patches backed out for some days. But current mainline failed to bootstrap today, apparently due to dereferencing freed memory in get_addr (3.4 didn't fail, but probably only because of disabled checks). Looks like an unrelated bug, though. Andreas.
Subject: Re: PR 13722 candidate fix On Fri, 2004-01-23 at 15:23, Zack Weinberg wrote: > unadjustible POST_MODIFY were being emitted in the wrong place). So, > if you wouldn't mind trying the appended revision (changes only > ia64.c)? FYI you got the PR number wrong again in the attachment file name. A -O0 c/ada bootstrap works. A -O1 c/ada bootstrap does not. So there is the possibility here of doing a binary search to narrow down the problem to one file, and with a little hackery, we could narrow down the problem to one function. This doesn't require any heavy lifting, but will be time consuming. I'm going to try disabling auto-inc addressing types, to see if maybe only one of them is broken. I think it would also be helpful to have debug_rtx() calls in ia64_split_tmode_move, so I can see the actual before and after rtx during the ada build. I might be able to spot a problem that way. I may try doing this.
Subject: Re: PR 13722 candidate fix I tried a bootstrap with all HAVE_POST_* macros turned off, and it still failed. I tried adding debug_rtx{_list,} calls to ia64_split_tmode_move and I noticed a few things. You are adding REG_INC notes when a MEM operand is a dest, but not when a src. An auto-inc address is still a side-effect even when it is used in a source operand. This is easy to fix by copying some code, I'll have to try another bootstrap with this change. I noticed that we are looking REG_*_P flag bits and alias info for registers, because you are generating new ones with with gen_rtx_REG without copying any info over from the old ones. It isn't clear if this is an optimization problem, I suspect not. I think this is only a potential performance loss.
Subject: Re: PR 13722 candidate fix My last bootstrap worked. I forgot to use the --enable-languages option, so I ended up doing a full build plus gnatlib_and_tools. I didn't try running the testsuite. I've got two changes on top of your patches, one to emit REG_INC notes if in[0] or in[1] contain an auto-inc address, and one that sets all of the HAVE_POST_* macros to zero which was added to help isolate the problem. I have removed the HAVE_POST_* changes, and I'm trying another bootstrap.
Subject: Re: PR 13722 candidate fix My latest bootstrap succeeded. I didn't run the testsuite, and I still had some debug code in there just to be safe, but I don't think either of those issues should matter. With the following patch on top of your patches, the IA-64 gcc port bootstrap again including Ada. This patch adds the REG_INC notes for auto-inc addresses in the IN operands. There is also the potential problem I pointed out earlier where we use INTVAL without checking to see if we have a CONST_INT. That isn't in this patch, but I think it should go in also. At this point, I would like to ask a favor. I have 3 IA-64 binutils bugs which need to be fixed, two of which are over a week old now, and 2 IA-64 gcc bugs that I need to respond to. I was hoping to work on them this week and/or weekend, but I ended up spending a lot of time helping you with your patch. I would appreciate being given some time to work on these problems without having to worry about the IA-64 compiler breaking. So what I am asking here is that if you check in a patch that breaks the IA-64 compiler, and you are not able to solve the problem yourself, that you revert the patch until I am able to help you. I am willing to help you with these kinds of things, but it isn't fair to others to let you monopolize my time, and it isn't fair to others to leave the IA-64 compiler broken for long periods of time. If you want to be really helpful, you could try looking at some of the IA-64 gcc bug reports in bugzilla. PR 7198 is on my list, and is one that you could handle.
Created attachment 5570 [details] pr13722.jw.diff
Subject: Re: PR 13722 candidate fix Jim Wilson <wilson@specifixinc.com> writes: > My latest bootstrap succeeded. I didn't run the testsuite, and I still > had some debug code in there just to be safe, but I don't think either > of those issues should matter. With the following patch on top of your > patches, the IA-64 gcc port bootstrap again including Ada. This patch > adds the REG_INC notes for auto-inc addresses in the IN operands. There > is also the potential problem I pointed out earlier where we use INTVAL > without checking to see if we have a CONST_INT. That isn't in this > patch, but I think it should go in also. Okay. I have incorporated your changes into my patch, added the CONST_INT check, and am running a bootstrap on ia64-hpux; if it succeeds I'll repost the combined patch. [...] > So what I am asking here is that if you check in a patch that > breaks the IA-64 compiler, and you are not able to solve the problem > yourself, that you revert the patch until I am able to help you. I am > willing to help you with these kinds of things, but it isn't fair to > others to let you monopolize my time, and it isn't fair to others to > leave the IA-64 compiler broken for long periods of time. I am willing to do this for most breaks. However, it is my understanding that breaks that affect only Ada are *not* considered critical, and in fact that there is no obligation for anyone to attempt to build the Ada front end. I normally do not bother building it even on platforms that can handle it (ia64-hpux cannot, as the runtime has not been ported). This is why I did not respond very fast to this bug report. I am willing to change my habits if the policy has changed, but not otherwise. > If you want to be really helpful, you could try looking at some of the > IA-64 gcc bug reports in bugzilla. PR 7198 is on my list, and is one > that you could handle. Sure, I'll look at that. zw
Subject: Re: [3.4/3.5 regression] [ia64] ICE in push_secondary_reload > However, it is my understanding that breaks that affect only Ada are > *not* considered critical, and in fact that there is no obligation for > anyone to attempt to build the Ada front end. I normally do not > bother building it even on platforms that can handle it (ia64-hpux > cannot, as the runtime has not been ported). This is why I did not Even if the run time has not been tailored for ia64-hpux, you can still build the generic part, which is more than enough for doing a bootstrap. > respond very fast to this bug report. I am willing to change my > habits if the policy has changed, but not otherwise. As shown by most of the Ada regressions recently, most of these regressions actually show real bugs in the non Ada part of GCC, so I believe this is in the interest of everyone to take Ada reports into consideration. I believe the policy you are stating was fine as long as Ada was not properly maintained, which is no longer the case. I'd suggest reconsidering the state of Ada, given the efforts invested recently on this front-end (as shown by the regular syncs and by the handling of Ada PRs in bugzilla). Arno
Subject: Hopefully final patch for PR 13722 This incorporates all bugfixes to date and gets good test results on ia64-hpux (3.4 branch): XPASS: gcc.c-torture/execute/990208-1.c execution, -O3 -fomit-frame-pointer XPASS: gcc.c-torture/execute/990208-1.c execution, -O3 -g FAIL: gcc.dg/compat/scalar-by-value-3 c_compat_x_tst.o-c_compat_y_tst.o execute FAIL: gcc.dg/compat/scalar-by-value-4 c_compat_x_tst.o-c_compat_y_tst.o execute FAIL: gcc.dg/compat/struct-by-value-5 c_compat_x_tst.o-c_compat_y_tst.o execute FAIL: gcc.dg/compat/struct-return-19 c_compat_x_tst.o-c_compat_y_tst.o execute FAIL: gcc.dg/20020219-1.c execution test FAIL: gcc.dg/20021014-1.c execution test FAIL: gcc.dg/20031202-1.c execution test FAIL: gcc.dg/cleanup-5.c (test for excess errors) FAIL: gcc.dg/nest.c execution test FAIL: gcc.dg/pch/inline-3.c -O0 -g assembly comparison FAIL: gcc.dg/pch/inline-3.c -O0 assembly comparison FAIL: gcc.dg/pch/inline-3.c -O1 assembly comparison FAIL: gcc.dg/pch/inline-3.c -O2 assembly comparison FAIL: gcc.dg/pch/inline-3.c -O3 -fomit-frame-pointer assembly comparison FAIL: gcc.dg/pch/inline-3.c -O3 -g assembly comparison FAIL: gcc.dg/pch/inline-3.c -Os assembly comparison I believe all of these are preexisting. All failures appear in both -milp32 and -mlp64 modes. zw =================================================================== Index: config/ia64/ia64.c --- config/ia64/ia64.c 16 Jan 2004 01:27:37 -0000 1.265 +++ config/ia64/ia64.c 26 Jan 2004 18:00:38 -0000 @@ -1362,62 +1362,37 @@ ia64_emit_cond_move (rtx op0, rtx op1, r } /* Split a post-reload TImode or TFmode reference into two DImode - components. */ + components. This is made extra difficult by the fact that we do + not get any scratch registers to work with, because reload cannot + be prevented from giving us a scratch that overlaps the register + pair involved. So instead, when addressing memory, we tweak the + pointer register up and back down with POST_INCs. Or up and not + back down when we can get away with it. + + REVERSED is true when the loads must be done in reversed order + (high word first) for correctness. DEAD is true when the pointer + dies with the second insn we generate and therefore the second + address must not carry a postmodify. + + May return an insn which is to be emitted after the moves. */ static rtx -ia64_split_tmode (rtx out[2], rtx in, rtx scratch) +ia64_split_tmode (rtx out[2], rtx in, bool reversed, bool dead) { + rtx fixup = 0; + switch (GET_CODE (in)) { case REG: - out[0] = gen_rtx_REG (DImode, REGNO (in)); - out[1] = gen_rtx_REG (DImode, REGNO (in) + 1); - return NULL_RTX; - - case MEM: - { - rtx base = XEXP (in, 0); - - switch (GET_CODE (base)) - { - case REG: - out[0] = adjust_address (in, DImode, 0); - break; - case POST_MODIFY: - base = XEXP (base, 0); - out[0] = adjust_address (in, DImode, 0); - break; - - /* Since we're changing the mode, we need to change to POST_MODIFY - as well to preserve the size of the increment. Either that or - do the update in two steps, but we've already got this scratch - register handy so let's use it. */ - case POST_INC: - base = XEXP (base, 0); - out[0] - = change_address (in, DImode, - gen_rtx_POST_MODIFY - (Pmode, base, plus_constant (base, 16))); - break; - case POST_DEC: - base = XEXP (base, 0); - out[0] - = change_address (in, DImode, - gen_rtx_POST_MODIFY - (Pmode, base, plus_constant (base, -16))); - break; - default: - abort (); - } - - if (scratch == NULL_RTX) - abort (); - out[1] = change_address (in, DImode, scratch); - return gen_adddi3 (scratch, base, GEN_INT (8)); - } + out[reversed] = gen_rtx_REG (DImode, REGNO (in)); + out[!reversed] = gen_rtx_REG (DImode, REGNO (in) + 1); + break; case CONST_INT: case CONST_DOUBLE: + /* Cannot occur reversed. */ + if (reversed) abort (); + if (GET_MODE (in) != TFmode) split_double (in, &out[0], &out[1]); else @@ -1444,11 +1419,108 @@ ia64_split_tmode (rtx out[2], rtx in, rt out[0] = GEN_INT (p[0]); out[1] = GEN_INT (p[1]); } - return NULL_RTX; + break; + + case MEM: + { + rtx base = XEXP (in, 0); + rtx offset; + + switch (GET_CODE (base)) + { + case REG: + if (!reversed) + { + out[0] = adjust_automodify_address + (in, DImode, gen_rtx_POST_INC (Pmode, base), 0); + out[1] = adjust_automodify_address + (in, DImode, dead ? 0 : gen_rtx_POST_DEC (Pmode, base), 8); + } + else + { + /* Reversal requires a pre-increment, which can only + be done as a separate insn. */ + emit_insn (gen_adddi3 (base, base, GEN_INT (8))); + out[0] = adjust_automodify_address + (in, DImode, gen_rtx_POST_DEC (Pmode, base), 8); + out[1] = adjust_address (in, DImode, 0); + } + break; + + case POST_INC: + if (reversed || dead) abort (); + /* Just do the increment in two steps. */ + out[0] = adjust_automodify_address (in, DImode, 0, 0); + out[1] = adjust_automodify_address (in, DImode, 0, 8); + break; + + case POST_DEC: + if (reversed || dead) abort (); + /* Add 8, subtract 24. */ + base = XEXP (base, 0); + out[0] = adjust_automodify_address + (in, DImode, gen_rtx_POST_INC (Pmode, base), 0); + out[1] = adjust_automodify_address + (in, DImode, + gen_rtx_POST_MODIFY (Pmode, base, plus_constant (base, -24)), + 8); + break; + + case POST_MODIFY: + if (reversed || dead) abort (); + /* Extract and adjust the modification. This case is + trickier than the others, because we might have an + index register, or we might have a combined offset that + doesn't fit a signed 9-bit displacement field. We can + assume the incoming expression is already legitimate. */ + offset = XEXP (base, 1); + base = XEXP (base, 0); + + out[0] = adjust_automodify_address + (in, DImode, gen_rtx_POST_INC (Pmode, base), 0); + + if (GET_CODE (XEXP (offset, 1)) == REG) + { + /* Can't adjust the postmodify to match. Emit the + original, then a separate addition insn. */ + out[1] = adjust_automodify_address (in, DImode, 0, 8); + fixup = gen_adddi3 (base, base, GEN_INT (-8)); + } + else if (GET_CODE (XEXP (offset, 1)) != CONST_INT) + abort (); + else if (INTVAL (XEXP (offset, 1)) < -256 + 8) + { + /* Again the postmodify cannot be made to match, but + in this case it's more efficient to get rid of the + postmodify entirely and fix up with an add insn. */ + out[1] = adjust_automodify_address (in, DImode, base, 8); + fixup = gen_adddi3 (base, base, + GEN_INT (INTVAL (XEXP (offset, 1)) - 8)); + } + else + { + /* Combined offset still fits in the displacement field. + (We cannot overflow it at the high end.) */ + out[1] = adjust_automodify_address + (in, DImode, + gen_rtx_POST_MODIFY (Pmode, base, + gen_rtx_PLUS (Pmode, base, + GEN_INT (INTVAL (XEXP (offset, 1)) - 8))), + 8); + } + break; + + default: + abort (); + } + break; + } default: abort (); } + + return fixup; } /* Split a TImode or TFmode move instruction after reload. @@ -1456,39 +1528,54 @@ ia64_split_tmode (rtx out[2], rtx in, rt void ia64_split_tmode_move (rtx operands[]) { - rtx adj1, adj2, in[2], out[2], insn; - int first; - - adj1 = ia64_split_tmode (in, operands[1], operands[2]); - adj2 = ia64_split_tmode (out, operands[0], operands[2]); - - first = 0; - if (reg_overlap_mentioned_p (out[0], in[1])) - { - if (reg_overlap_mentioned_p (out[1], in[0])) - abort (); - first = 1; - } - - if (adj1 && adj2) - abort (); - if (adj1) - emit_insn (adj1); - if (adj2) - emit_insn (adj2); - insn = emit_insn (gen_rtx_SET (VOIDmode, out[first], in[first])); - if (GET_CODE (out[first]) == MEM - && GET_CODE (XEXP (out[first], 0)) == POST_MODIFY) - REG_NOTES (insn) = gen_rtx_EXPR_LIST (REG_INC, - XEXP (XEXP (out[first], 0), 0), - REG_NOTES (insn)); - insn = emit_insn (gen_rtx_SET (VOIDmode, out[!first], in[!first])); - if (GET_CODE (out[!first]) == MEM - && GET_CODE (XEXP (out[!first], 0)) == POST_MODIFY) - REG_NOTES (insn) = gen_rtx_EXPR_LIST (REG_INC, - XEXP (XEXP (out[!first], 0), 0), - REG_NOTES (insn)); + rtx in[2], out[2], insn; + rtx fixup[2]; + bool dead = false; + bool reversed = false; + + /* It is possible for reload to decide to overwrite a pointer with + the value it points to. In that case we have to do the loads in + the appropriate order so that the pointer is not destroyed too + early. Also we must not generate a postmodify for that second + load, or rws_access_regno will abort. */ + if (GET_CODE (operands[1]) == MEM + && reg_overlap_mentioned_p (operands[0], operands[1])) + { + rtx base = XEXP (operands[1], 0); + while (GET_CODE (base) != REG) + base = XEXP (base, 0); + + if (REGNO (base) == REGNO (operands[0])) + reversed = true; + dead = true; + } + + fixup[0] = ia64_split_tmode (in, operands[1], reversed, dead); + fixup[1] = ia64_split_tmode (out, operands[0], reversed, dead); + +#define MAYBE_ADD_REG_INC_NOTE(INSN, EXP) \ + if (GET_CODE (EXP) == MEM \ + && (GET_CODE (XEXP (EXP, 0)) == POST_MODIFY \ + || GET_CODE (XEXP (EXP, 0)) == POST_INC \ + || GET_CODE (XEXP (EXP, 0)) == POST_DEC)) \ + REG_NOTES (INSN) = gen_rtx_EXPR_LIST (REG_INC, \ + XEXP (XEXP (EXP, 0), 0), \ + REG_NOTES (INSN)) + + insn = emit_insn (gen_rtx_SET (VOIDmode, out[0], in[0])); + MAYBE_ADD_REG_INC_NOTE (insn, in[0]); + MAYBE_ADD_REG_INC_NOTE (insn, out[0]); + + insn = emit_insn (gen_rtx_SET (VOIDmode, out[1], in[1])); + MAYBE_ADD_REG_INC_NOTE (insn, in[1]); + MAYBE_ADD_REG_INC_NOTE (insn, out[1]); + + if (fixup[0]) + emit_insn (fixup[0]); + if (fixup[1]) + emit_insn (fixup[1]); +#undef MAYBE_ADD_REG_INC_NOTE } /* ??? Fixing GR->FR XFmode moves during reload is hard. You need to go @@ -4451,13 +4538,6 @@ ia64_secondary_reload_class (enum reg_cl /* This can happen when we take a BImode subreg of a DImode value, and that DImode value winds up in some non-GR register. */ if (regno >= 0 && ! GENERAL_REGNO_P (regno) && ! PR_REGNO_P (regno)) - return GR_REGS; - break; - - case GR_REGS: - /* Since we have no offsettable memory addresses, we need a temporary - to hold the address of the second word. */ - if (mode == TImode || mode == TFmode) return GR_REGS; break; =================================================================== Index: config/ia64/ia64.md --- config/ia64/ia64.md 16 Jan 2004 01:27:38 -0000 1.119 +++ config/ia64/ia64.md 26 Jan 2004 18:00:38 -0000 @@ -584,11 +584,12 @@ [(set_attr "itanium_class" "ialu")]) ;; With no offsettable memory references, we've got to have a scratch -;; around to play with the second word. +;; around to play with the second word. However, in order to avoid a +;; reload nightmare we lie, claim we don't need one, and fix it up +;; in ia64_split_tmode_move. (define_expand "movti" - [(parallel [(set (match_operand:TI 0 "general_operand" "") - (match_operand:TI 1 "general_operand" "")) - (clobber (match_scratch:DI 2 ""))])] + [(set (match_operand:TI 0 "general_operand" "") + (match_operand:TI 1 "general_operand" ""))] "" { rtx op1 = ia64_expand_move (operands[0], operands[1]); @@ -599,8 +600,7 @@ (define_insn_and_split "*movti_internal" [(set (match_operand:TI 0 "nonimmediate_operand" "=r,r,m") - (match_operand:TI 1 "general_operand" "ri,m,r")) - (clobber (match_scratch:DI 2 "=X,&r,&r"))] + (match_operand:TI 1 "general_operand" "ri,m,r"))] "ia64_move_ok (operands[0], operands[1])" "#" "reload_completed" @@ -612,20 +612,6 @@ [(set_attr "itanium_class" "unknown") (set_attr "predicable" "no")]) -(define_expand "reload_inti" - [(parallel [(set (match_operand:TI 0 "register_operand" "=r") - (match_operand:TI 1 "memory_operand" "m")) - (clobber (match_operand:DI 2 "register_operand" "=&r"))])] - "" - "") - -(define_expand "reload_outti" - [(parallel [(set (match_operand:TI 0 "memory_operand" "=m") - (match_operand:TI 1 "register_operand" "r")) - (clobber (match_operand:DI 2 "register_operand" "=&r"))])] - "" - "") - ;; Floating Point Moves ;; ;; Note - Patterns for SF mode moves are compulsory, but @@ -764,13 +750,10 @@ [(set_attr "itanium_class" "fmisc,fld,stf")]) ;; Better code generation via insns that deal with TFmode register pairs -;; directly. -;; With no offsettable memory references, we've got to have a scratch -;; around to play with the second word. +;; directly. Same concerns apply as for TImode. (define_expand "movtf" - [(parallel [(set (match_operand:TF 0 "general_operand" "") - (match_operand:TF 1 "general_operand" "")) - (clobber (match_scratch:DI 2 ""))])] + [(set (match_operand:TF 0 "general_operand" "") + (match_operand:TF 1 "general_operand" ""))] "" { rtx op1 = ia64_expand_move (operands[0], operands[1]); @@ -781,8 +764,7 @@ (define_insn_and_split "*movtf_internal" [(set (match_operand:TF 0 "nonimmediate_operand" "=r,r,m") - (match_operand:TF 1 "general_operand" "ri,m,r")) - (clobber (match_scratch:DI 2 "=X,&r,&r"))] + (match_operand:TF 1 "general_operand" "ri,m,r"))] "ia64_move_ok (operands[0], operands[1])" "#" "reload_completed" @@ -794,19 +776,6 @@ [(set_attr "itanium_class" "unknown") (set_attr "predicable" "no")]) -(define_expand "reload_intf" - [(parallel [(set (match_operand:TF 0 "register_operand" "=r") - (match_operand:TF 1 "memory_operand" "m")) - (clobber (match_operand:DI 2 "register_operand" "=&r"))])] - "" - "") - -(define_expand "reload_outtf" - [(parallel [(set (match_operand:TF 0 "memory_operand" "=m") - (match_operand:TF 1 "register_operand" "r")) - (clobber (match_operand:DI 2 "register_operand" "=&r"))])] - "" - "") ;; :::::::::::::::::::: ;; ::
Subject: Re: Hopefully final patch for PR 13722 On Mon, 2004-01-26 at 10:02, Zack Weinberg wrote: > This incorporates all bugfixes to date and gets good test results on > ia64-hpux (3.4 branch): Thanks. It looks good to me also. I'll add in your patches for 13722 and 7198 and do another ia64-linux bootstrap. My IA-64 machine is idle today anyways, so this is no bother for me. I don't mind if you go ahead and check in the patches first. I'll let you know if something goes wrong.
Subject: Re: Hopefully final patch for PR 13722 I probably should have spoken up before but I was out of town for a while. I've got an ia64 machine that I try to run nightly bootstrap and check on (it's sporadic right now). I'd be glad to get it so it does run nightly and send out the results (and try to at least isolate what breaks). This would be for ia64-linux, on Debian unstable. Are there particular configuration parameters that should be set? I typically only build and test for C, C++ and g77, without maintainer mode set. Any particular version of the compiler to be used for the bootstrap? What should I do with the results? Email them somewhere? Post them somewhere on gcc.gnu.org? I've only done a real cursory search of the site but didn't find much.... Let me know what I can do to help. On Mon, 2004-01-26 at 17:08, Jim Wilson wrote: > On Mon, 2004-01-26 at 10:02, Zack Weinberg wrote: > > This incorporates all bugfixes to date and gets good test results on > > ia64-hpux (3.4 branch): > > Thanks. It looks good to me also. I'll add in your patches for 13722 > and 7198 and do another ia64-linux bootstrap. My IA-64 machine is idle > today anyways, so this is no bother for me. I don't mind if you go > ahead and check in the patches first. I'll let you know if something > goes wrong. -- Ciao, al ---------------------------- Al Stone Linux & Open Source Lab Hewlett-Packard Company Phone: 970-898-0345 Telnet: 898-0345 Fax: 970-898-3804 E-mail: ahs3@fc.hp.com ----------------------------
Subject: Re: Hopefully final patch for PR 13722 Al Stone <ahs3@fc.hp.com> writes: > I probably should have spoken up before but I was out of town > for a while. > > I've got an ia64 machine that I try to run nightly bootstrap > and check on (it's sporadic right now). I'd be glad to get > it so it does run nightly and send out the results (and try > to at least isolate what breaks). This would be for ia64-linux, > on Debian unstable. That would be really great. > Are there particular configuration parameters that should be > set? I typically only build and test for C, C++ and g77, without > maintainer mode set. That should be fine. > Any particular version of the compiler to be used for the bootstrap? I've been using 3.3.2 for ia64 bootstrap base compiler, with good results. > What should I do with the results? Email them somewhere? Post > them somewhere on gcc.gnu.org? I've only done a real cursory > search of the site but didn't find much.... There is a mailing list, gcc-testresults, for posting these results. If you set up an automatic nag script that reports regressions when they happen, there is also gcc-regression. Talk to Geoff Keating and Phil Edwards about this. zw
Subject: Re: Hopefully final patch for PR 13722 On Tue, 2004-01-27 at 11:13, Zack Weinberg wrote: > Al Stone <ahs3@fc.hp.com> writes: > > I've got an ia64 machine that I try to run nightly bootstrap > > and check on (it's sporadic right now). I'd be glad to get > > it so it does run nightly and send out the results (and try > > to at least isolate what breaks). This would be for ia64-linux, > > on Debian unstable. > > That would be really great. Cool. I'll try to get it running by this weekend. > > Any particular version of the compiler to be used for the bootstrap? > > I've been using 3.3.2 for ia64 bootstrap base compiler, with good results. Hmm. The Debian one is usually a _little_ bit different than the officially released version. I'll start with using the Debian default (3.3.3 today) and we'll see what happens. > > What should I do with the results? Email them somewhere? Post > > them somewhere on gcc.gnu.org? I've only done a real cursory > > search of the site but didn't find much.... > > There is a mailing list, gcc-testresults, for posting these results. Ah. Thanks. I knew there was something somewhere but just wasn't seeing it. > If you set up an automatic nag script that reports regressions when > they happen, there is also gcc-regression. Talk to Geoff Keating > and Phil Edwards about this. I sent a note off to Geoff; thanks for the pointer. I'll try to get this running in the next few days, too. -- Ciao, al ---------------------------- Al Stone Linux & Open Source Lab Hewlett-Packard Company Phone: 970-898-0345 Telnet: 898-0345 Fax: 970-898-3804 E-mail: ahs3@fc.hp.com ----------------------------
Subject: Re: Hopefully final patch for PR 13722 Al Stone <ahs3@fc.hp.com> writes: >> > Any particular version of the compiler to be used for the bootstrap? >> >> I've been using 3.3.2 for ia64 bootstrap base compiler, with good results. > > Hmm. The Debian one is usually a _little_ bit different than > the officially released version. I'll start with using the Debian > default (3.3.3 today) and we'll see what happens. It shouldn't really matter much, as long as it is not too buggy. Andreas.
Subject: Final patch for PR 13722 The only change from the previous patch is, ia64_split_tmode_move now correctly handles a reg-reg move where the destination overlaps the source by one register - e.g. (set (reg:TF 15) (reg:TF 14)). This cures the regression of compat/scalar-by-value-3. [ This is not an ABI issue; it was a straight miscompilation, which escaped all other notice because such moves tend to get deleted in optimizing compilation, before they ever get this far. Also, the compat tests involving complex numbers have some very strange code in them. "long double complex n = (8.0,9.0);" sets n to the complex number 9+0i, *not* 8+9i. I do not think this was the intent. ] Bootstrapped ia64-hpux (C, C++); applied mainline and 3.4 branch. I am also checking the patch for scalar-by-value-4 onto the 3.4 branch at this time. zw 2004-01-28 Zack Weinberg <zack@codesourcery.com> Jim Wilson <wilson@specifixinc.com> * config/ia64/ia64.c (ia64_split_tmode, ia64_split_tmode_move): Rewrite to use POST_INC/POST_DEC/POST_MODIFY instead of a scratch pointer. (ia64_secondary_reload_class): Delete case GR_REGS. * config/ia64/ia64.md (movti, *movti_internal, movtf, *movtf_internal): Do not allocate a scratch register. (reload_inti, reload_outti, reload_intf, reload_outtf): Delete. =================================================================== Index: config/ia64/ia64.c --- config/ia64/ia64.c 27 Jan 2004 22:48:11 -0000 1.267 +++ config/ia64/ia64.c 28 Jan 2004 17:45:39 -0000 @@ -1395,62 +1395,37 @@ ia64_emit_cond_move (rtx op0, rtx op1, r } /* Split a post-reload TImode or TFmode reference into two DImode - components. */ + components. This is made extra difficult by the fact that we do + not get any scratch registers to work with, because reload cannot + be prevented from giving us a scratch that overlaps the register + pair involved. So instead, when addressing memory, we tweak the + pointer register up and back down with POST_INCs. Or up and not + back down when we can get away with it. + + REVERSED is true when the loads must be done in reversed order + (high word first) for correctness. DEAD is true when the pointer + dies with the second insn we generate and therefore the second + address must not carry a postmodify. + + May return an insn which is to be emitted after the moves. */ static rtx -ia64_split_tmode (rtx out[2], rtx in, rtx scratch) +ia64_split_tmode (rtx out[2], rtx in, bool reversed, bool dead) { + rtx fixup = 0; + switch (GET_CODE (in)) { case REG: - out[0] = gen_rtx_REG (DImode, REGNO (in)); - out[1] = gen_rtx_REG (DImode, REGNO (in) + 1); - return NULL_RTX; - - case MEM: - { - rtx base = XEXP (in, 0); - - switch (GET_CODE (base)) - { - case REG: - out[0] = adjust_address (in, DImode, 0); - break; - case POST_MODIFY: - base = XEXP (base, 0); - out[0] = adjust_address (in, DImode, 0); - break; - - /* Since we're changing the mode, we need to change to POST_MODIFY - as well to preserve the size of the increment. Either that or - do the update in two steps, but we've already got this scratch - register handy so let's use it. */ - case POST_INC: - base = XEXP (base, 0); - out[0] - = change_address (in, DImode, - gen_rtx_POST_MODIFY - (Pmode, base, plus_constant (base, 16))); - break; - case POST_DEC: - base = XEXP (base, 0); - out[0] - = change_address (in, DImode, - gen_rtx_POST_MODIFY - (Pmode, base, plus_constant (base, -16))); - break; - default: - abort (); - } - - if (scratch == NULL_RTX) - abort (); - out[1] = change_address (in, DImode, scratch); - return gen_adddi3 (scratch, base, GEN_INT (8)); - } + out[reversed] = gen_rtx_REG (DImode, REGNO (in)); + out[!reversed] = gen_rtx_REG (DImode, REGNO (in) + 1); + break; case CONST_INT: case CONST_DOUBLE: + /* Cannot occur reversed. */ + if (reversed) abort (); + if (GET_MODE (in) != TFmode) split_double (in, &out[0], &out[1]); else @@ -1477,11 +1452,108 @@ ia64_split_tmode (rtx out[2], rtx in, rt out[0] = GEN_INT (p[0]); out[1] = GEN_INT (p[1]); } - return NULL_RTX; + break; + + case MEM: + { + rtx base = XEXP (in, 0); + rtx offset; + + switch (GET_CODE (base)) + { + case REG: + if (!reversed) + { + out[0] = adjust_automodify_address + (in, DImode, gen_rtx_POST_INC (Pmode, base), 0); + out[1] = adjust_automodify_address + (in, DImode, dead ? 0 : gen_rtx_POST_DEC (Pmode, base), 8); + } + else + { + /* Reversal requires a pre-increment, which can only + be done as a separate insn. */ + emit_insn (gen_adddi3 (base, base, GEN_INT (8))); + out[0] = adjust_automodify_address + (in, DImode, gen_rtx_POST_DEC (Pmode, base), 8); + out[1] = adjust_address (in, DImode, 0); + } + break; + + case POST_INC: + if (reversed || dead) abort (); + /* Just do the increment in two steps. */ + out[0] = adjust_automodify_address (in, DImode, 0, 0); + out[1] = adjust_automodify_address (in, DImode, 0, 8); + break; + + case POST_DEC: + if (reversed || dead) abort (); + /* Add 8, subtract 24. */ + base = XEXP (base, 0); + out[0] = adjust_automodify_address + (in, DImode, gen_rtx_POST_INC (Pmode, base), 0); + out[1] = adjust_automodify_address + (in, DImode, + gen_rtx_POST_MODIFY (Pmode, base, plus_constant (base, -24)), + 8); + break; + + case POST_MODIFY: + if (reversed || dead) abort (); + /* Extract and adjust the modification. This case is + trickier than the others, because we might have an + index register, or we might have a combined offset that + doesn't fit a signed 9-bit displacement field. We can + assume the incoming expression is already legitimate. */ + offset = XEXP (base, 1); + base = XEXP (base, 0); + + out[0] = adjust_automodify_address + (in, DImode, gen_rtx_POST_INC (Pmode, base), 0); + + if (GET_CODE (XEXP (offset, 1)) == REG) + { + /* Can't adjust the postmodify to match. Emit the + original, then a separate addition insn. */ + out[1] = adjust_automodify_address (in, DImode, 0, 8); + fixup = gen_adddi3 (base, base, GEN_INT (-8)); + } + else if (GET_CODE (XEXP (offset, 1)) != CONST_INT) + abort (); + else if (INTVAL (XEXP (offset, 1)) < -256 + 8) + { + /* Again the postmodify cannot be made to match, but + in this case it's more efficient to get rid of the + postmodify entirely and fix up with an add insn. */ + out[1] = adjust_automodify_address (in, DImode, base, 8); + fixup = gen_adddi3 (base, base, + GEN_INT (INTVAL (XEXP (offset, 1)) - 8)); + } + else + { + /* Combined offset still fits in the displacement field. + (We cannot overflow it at the high end.) */ + out[1] = adjust_automodify_address + (in, DImode, + gen_rtx_POST_MODIFY (Pmode, base, + gen_rtx_PLUS (Pmode, base, + GEN_INT (INTVAL (XEXP (offset, 1)) - 8))), + 8); + } + break; + + default: + abort (); + } + break; + } default: abort (); } + + return fixup; } /* Split a TImode or TFmode move instruction after reload. @@ -1489,39 +1561,60 @@ ia64_split_tmode (rtx out[2], rtx in, rt void ia64_split_tmode_move (rtx operands[]) { - rtx adj1, adj2, in[2], out[2], insn; - int first; - - adj1 = ia64_split_tmode (in, operands[1], operands[2]); - adj2 = ia64_split_tmode (out, operands[0], operands[2]); - - first = 0; - if (reg_overlap_mentioned_p (out[0], in[1])) - { - if (reg_overlap_mentioned_p (out[1], in[0])) - abort (); - first = 1; - } - - if (adj1 && adj2) - abort (); - if (adj1) - emit_insn (adj1); - if (adj2) - emit_insn (adj2); - insn = emit_insn (gen_rtx_SET (VOIDmode, out[first], in[first])); - if (GET_CODE (out[first]) == MEM - && GET_CODE (XEXP (out[first], 0)) == POST_MODIFY) - REG_NOTES (insn) = gen_rtx_EXPR_LIST (REG_INC, - XEXP (XEXP (out[first], 0), 0), - REG_NOTES (insn)); - insn = emit_insn (gen_rtx_SET (VOIDmode, out[!first], in[!first])); - if (GET_CODE (out[!first]) == MEM - && GET_CODE (XEXP (out[!first], 0)) == POST_MODIFY) - REG_NOTES (insn) = gen_rtx_EXPR_LIST (REG_INC, - XEXP (XEXP (out[!first], 0), 0), - REG_NOTES (insn)); + rtx in[2], out[2], insn; + rtx fixup[2]; + bool dead = false; + bool reversed = false; + + /* It is possible for reload to decide to overwrite a pointer with + the value it points to. In that case we have to do the loads in + the appropriate order so that the pointer is not destroyed too + early. Also we must not generate a postmodify for that second + load, or rws_access_regno will abort. */ + if (GET_CODE (operands[1]) == MEM + && reg_overlap_mentioned_p (operands[0], operands[1])) + { + rtx base = XEXP (operands[1], 0); + while (GET_CODE (base) != REG) + base = XEXP (base, 0); + + if (REGNO (base) == REGNO (operands[0])) + reversed = true; + dead = true; + } + /* Another reason to do the moves in reversed order is if the first + element of the target register pair is also the second element of + the source register pair. */ + if (GET_CODE (operands[0]) == REG && GET_CODE (operands[1]) == REG + && REGNO (operands[0]) == REGNO (operands[1]) + 1) + reversed = true; + + fixup[0] = ia64_split_tmode (in, operands[1], reversed, dead); + fixup[1] = ia64_split_tmode (out, operands[0], reversed, dead); + +#define MAYBE_ADD_REG_INC_NOTE(INSN, EXP) \ + if (GET_CODE (EXP) == MEM \ + && (GET_CODE (XEXP (EXP, 0)) == POST_MODIFY \ + || GET_CODE (XEXP (EXP, 0)) == POST_INC \ + || GET_CODE (XEXP (EXP, 0)) == POST_DEC)) \ + REG_NOTES (INSN) = gen_rtx_EXPR_LIST (REG_INC, \ + XEXP (XEXP (EXP, 0), 0), \ + REG_NOTES (INSN)) + + insn = emit_insn (gen_rtx_SET (VOIDmode, out[0], in[0])); + MAYBE_ADD_REG_INC_NOTE (insn, in[0]); + MAYBE_ADD_REG_INC_NOTE (insn, out[0]); + + insn = emit_insn (gen_rtx_SET (VOIDmode, out[1], in[1])); + MAYBE_ADD_REG_INC_NOTE (insn, in[1]); + MAYBE_ADD_REG_INC_NOTE (insn, out[1]); + + if (fixup[0]) + emit_insn (fixup[0]); + if (fixup[1]) + emit_insn (fixup[1]); +#undef MAYBE_ADD_REG_INC_NOTE } /* ??? Fixing GR->FR XFmode moves during reload is hard. You need to go @@ -4489,13 +4582,6 @@ ia64_secondary_reload_class (enum reg_cl /* This can happen when we take a BImode subreg of a DImode value, and that DImode value winds up in some non-GR register. */ if (regno >= 0 && ! GENERAL_REGNO_P (regno) && ! PR_REGNO_P (regno)) - return GR_REGS; - break; - - case GR_REGS: - /* Since we have no offsettable memory addresses, we need a temporary - to hold the address of the second word. */ - if (mode == TImode || mode == TFmode) return GR_REGS; break; =================================================================== Index: config/ia64/ia64.md --- config/ia64/ia64.md 27 Jan 2004 17:42:59 -0000 1.120 +++ config/ia64/ia64.md 28 Jan 2004 17:45:40 -0000 @@ -584,11 +584,12 @@ [(set_attr "itanium_class" "ialu")]) ;; With no offsettable memory references, we've got to have a scratch -;; around to play with the second word. +;; around to play with the second word. However, in order to avoid a +;; reload nightmare we lie, claim we don't need one, and fix it up +;; in ia64_split_tmode_move. (define_expand "movti" - [(parallel [(set (match_operand:TI 0 "general_operand" "") - (match_operand:TI 1 "general_operand" "")) - (clobber (match_scratch:DI 2 ""))])] + [(set (match_operand:TI 0 "general_operand" "") + (match_operand:TI 1 "general_operand" ""))] "" { rtx op1 = ia64_expand_move (operands[0], operands[1]); @@ -599,8 +600,7 @@ (define_insn_and_split "*movti_internal" [(set (match_operand:TI 0 "nonimmediate_operand" "=r,r,m") - (match_operand:TI 1 "general_operand" "ri,m,r")) - (clobber (match_scratch:DI 2 "=X,&r,&r"))] + (match_operand:TI 1 "general_operand" "ri,m,r"))] "ia64_move_ok (operands[0], operands[1])" "#" "reload_completed" @@ -612,20 +612,6 @@ [(set_attr "itanium_class" "unknown") (set_attr "predicable" "no")]) -(define_expand "reload_inti" - [(parallel [(set (match_operand:TI 0 "register_operand" "=r") - (match_operand:TI 1 "memory_operand" "m")) - (clobber (match_operand:DI 2 "register_operand" "=&r"))])] - "" - "") - -(define_expand "reload_outti" - [(parallel [(set (match_operand:TI 0 "memory_operand" "=m") - (match_operand:TI 1 "register_operand" "r")) - (clobber (match_operand:DI 2 "register_operand" "=&r"))])] - "" - "") - ;; Floating Point Moves ;; ;; Note - Patterns for SF mode moves are compulsory, but @@ -764,13 +750,10 @@ [(set_attr "itanium_class" "fmisc,fld,stf")]) ;; Better code generation via insns that deal with TFmode register pairs -;; directly. -;; With no offsettable memory references, we've got to have a scratch -;; around to play with the second word. +;; directly. Same concerns apply as for TImode. (define_expand "movtf" - [(parallel [(set (match_operand:TF 0 "general_operand" "") - (match_operand:TF 1 "general_operand" "")) - (clobber (match_scratch:DI 2 ""))])] + [(set (match_operand:TF 0 "general_operand" "") + (match_operand:TF 1 "general_operand" ""))] "" { rtx op1 = ia64_expand_move (operands[0], operands[1]); @@ -781,8 +764,7 @@ (define_insn_and_split "*movtf_internal" [(set (match_operand:TF 0 "nonimmediate_operand" "=r,r,m") - (match_operand:TF 1 "general_operand" "ri,m,r")) - (clobber (match_scratch:DI 2 "=X,&r,&r"))] + (match_operand:TF 1 "general_operand" "ri,m,r"))] "ia64_move_ok (operands[0], operands[1])" "#" "reload_completed" @@ -794,19 +776,6 @@ [(set_attr "itanium_class" "unknown") (set_attr "predicable" "no")]) -(define_expand "reload_intf" - [(parallel [(set (match_operand:TF 0 "register_operand" "=r") - (match_operand:TF 1 "memory_operand" "m")) - (clobber (match_operand:DI 2 "register_operand" "=&r"))])] - "" - "") - -(define_expand "reload_outtf" - [(parallel [(set (match_operand:TF 0 "memory_operand" "=m") - (match_operand:TF 1 "register_operand" "r")) - (clobber (match_operand:DI 2 "register_operand" "=&r"))])] - "" - "") ;; :::::::::::::::::::: ;; ::
Subject: Re: Hopefully final patch for PR 13722 Zack Weinberg writes: > Al Stone <ahs3@fc.hp.com> writes: > > > I probably should have spoken up before but I was out of town > > for a while. > > > > I've got an ia64 machine that I try to run nightly bootstrap > > and check on (it's sporadic right now). I'd be glad to get > > it so it does run nightly and send out the results (and try > > to at least isolate what breaks). This would be for ia64-linux, > > on Debian unstable. > > That would be really great. > > > Are there particular configuration parameters that should be > > set? I typically only build and test for C, C++ and g77, without > > maintainer mode set. > > That should be fine. > > > Any particular version of the compiler to be used for the bootstrap? > > I've been using 3.3.2 for ia64 bootstrap base compiler, with good results. CVS 20040128 including your patch fails in the first ada file in stage2 (Debian unstable): stage1/xgcc -Bstage1/ -B/usr/lib/gcc-snapshot/ia64-linux/bin/ -O2 -DIN_GCC -W -Wall -Wwrite-strings -Wstrict-prototypes -Wmissing-prototypes -pedantic -Wno-long-long -Wold-style-definition -DHAVE_CONFIG_H gcov-dump.o version.o ../libiberty/libiberty.a -o gcov-dump stage1/xgcc -Bstage1/ -B/usr/lib/gcc-snapshot/ia64-linux/bin/ -c -O2 -gnatpg -gnata -I- -I. -Iada -I../../src/gcc/ada ../../src/gcc/ada/ada.ads -o ada/ada.o xgcc: Internal error: Illegal instruction (program gnat1) Please submit a full bug report. See <URL:http://gcc.gnu.org/bugs.html> for instructions. make[4]: *** [ada/ada.o] Error 1 make[4]: Leaving directory `/home/doko/gcc/snap/gcc-snapshot-20040128/build/gcc' make[3]: *** [stage2_build] Error 2
Subject: Re: Hopefully final patch for PR 13722 On Wed, 2004-01-28 at 12:02, Matthias Klose wrote: > CVS 20040128 including your patch fails in the first ada file in > stage2 (Debian unstable): I couldn't reproduce this on debian testing using a gcc tree checked out today. It successfully compiled the stage2 ada/ada.o file. It later died because of a dwarf2out.c bug on ada/osint.o, but that is an unrelated problem. I have debian unstable on a second partition, so I could look at this later if I have to.
believed fixed, mainline and branch.
Subject: Re: Hopefully final patch for PR 13722 On Wed, 2004-01-28 at 12:02, Matthias Klose wrote: > CVS 20040128 including your patch fails in the first ada file in > stage2 (Debian unstable): Keep in mind that since the gcc-3.4 branch was created 2 weeks ago, mainline gcc is now a free for all, and it is likely that gcc will be frequently broken. If you want a stable compiler, use the gcc-3.4 branch instead of mainline. I didn't reproduce the exact problem you saw, but I got an ICE compiling c-decl.c when I tried a build without Ada. This is failing in alias.c get_addr() as called from the scheduler, because CSE_LIB_PTR returns NULL, and then we try to dereference it. This problem I just saw is unrelated to Zack's patch. This could be the same problem you saw. The cselib issue is one of accessing memory after it has been freed, and it is known to be sensitive to the environment size, i.e. you get different failures in different files depending on your environment size.
Subject: Re: Hopefully final patch for PR 13722 Jim Wilson writes: > On Wed, 2004-01-28 at 12:02, Matthias Klose wrote: > > CVS 20040128 including your patch fails in the first ada file in > > stage2 (Debian unstable): > > Keep in mind that since the gcc-3.4 branch was created 2 weeks ago, > mainline gcc is now a free for all, and it is likely that gcc will be > frequently broken. If you want a stable compiler, use the gcc-3.4 > branch instead of mainline. sorry, I was a bit unspecific. this was the 3.4 branch. last checkin 2004-01-28 Zack Weinberg <zack@codesourcery.com> * config/ia64/ia64.md (fetchadd_acq_si, fetchadd_acq_di) (cmpxchg_acq_si, cmpxchg_acq_di): Exchange match_dup and match_operand expressions so that all match_dups appear lexically after their corresponding match_operands.
Subject: Re: Hopefully final patch for PR 13722 On Wed, 2004-01-28 at 22:41, Matthias Klose wrote: > sorry, I was a bit unspecific. this was the 3.4 branch. last checkin I have a copy of the gcc-3.4 branch updated today. make bootstrap worked on debian testing (last updated Nov?), debian unstable (last updated Dec?), and debian unstable (current as of today). I ran apt-get in between the last two tries.