* More fp bug in egcs @ 1998-04-25 18:25 H.J. Lu 1998-04-27 21:29 ` Jim Wilson 1998-04-30 20:03 ` Jim Wilson 0 siblings, 2 replies; 22+ messages in thread From: H.J. Lu @ 1998-04-25 18:25 UTC (permalink / raw) To: wilson, law, scox, crux; +Cc: egcs Hi, I discovered this x86 fp bug by accident. # gcc -B/home/work/gnu/bin/egcs/gcc/ -S mpgeomnn.c gcc: Internal compiler error: program cc1 got fatal signal 6 It is very similar to the one fixed by Jim. It will compile if -O is used. I think it may have something to do with Jim's patch. In the greg rtl dump, the clobbered register is used as input in the next insn. Thanks. -- H.J. Lu (hjl@gnu.org) ---mpgeomnn.c--- typedef struct _geom_elem { double coeffs[6]; double constant[3 ]; int do_band[3]; } pGeomDefRec, *pGeomDefPtr; typedef struct _mpgeombanddef { double first_mlow, first_mhigh; int first_ilow, first_ihigh; double *s_locs; int *x_locs; int x_start; int x_end; int int_constant; int yOut; int out_width; int out_height; int in_width; int in_height; int lo_src_available; int hi_src_available; void (*linefunc) (); void (*fillfunc) (); } mpGeometryBandRec, *mpGeometryBandPtr; typedef void *pointer; typedef unsigned char CARD8; typedef CARD8 BytePixel; static void BiGL_B (OUTP,srcimg,width,sline,pedpvt,pvtband) pointer OUTP; pointer *srcimg; register int width; int sline; pGeomDefPtr pedpvt; mpGeometryBandPtr pvtband; { register float s, t, st; register double a = pedpvt->coeffs[0]; register double c = pedpvt->coeffs[2]; register double srcpix = a * ((double)(0.0000)) + pedpvt->coeffs[1] * (pvtband->yOut + ((double)(0.0000)) ) + pedpvt->coeffs[4]; register double srcline = c * ((double)(0.0000)) + pedpvt->coeffs[3] * (pvtband->yOut + ((double)(0.0000)) ) + pedpvt->coeffs[5]; register int isrcline,isrcpix; register BytePixel constant = ( BytePixel ) pvtband-> int_constant ; register BytePixel *outp = ( BytePixel *) OUTP; register BytePixel *ptrIn, *ptrJn; register BytePixel val; register int srcwidth = pvtband->in_width - 1; register int minline = pvtband->lo_src_available; register int maxline = pvtband->hi_src_available; while ( width > 0 ) { isrcline = srcline; isrcpix = srcpix; val = constant; if ( (isrcline >= minline) && (isrcline < maxline) ) { s = srcpix - isrcpix; ptrIn = ( BytePixel *) srcimg[isrcline]; t = srcline - isrcline; ptrJn = ( BytePixel *) srcimg[isrcline+1]; st = s * t; if ( (isrcpix >= 0) && (isrcpix < srcwidth) ) val = ptrIn[isrcpix] * ((float)1. - s - t + st) + ptrIn[isrcpix+1] * (s - st) + ptrJn[isrcpix] * (t - st) + ptrJn[isrcpix+1] * (st) + (float)0.5 ; } width--; srcline += c; srcpix += a; *outp++ = val; } } ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: More fp bug in egcs 1998-04-25 18:25 More fp bug in egcs H.J. Lu @ 1998-04-27 21:29 ` Jim Wilson 1998-04-30 20:03 ` Jim Wilson 1 sibling, 0 replies; 22+ messages in thread From: Jim Wilson @ 1998-04-27 21:29 UTC (permalink / raw) To: H.J. Lu; +Cc: law, scox, crux, egcs Yes, this is a problem with my patch. My patch that makes the SF/DF->DI conversions work with -O makes them fail without -O. I missed that. Grr. I am going to have to take another look at this problem. I believe this patch was added to the pre-release 1.0.3, and hence we may have a serious problem here. There is only a problem if people compile without optimization though. Jim ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: More fp bug in egcs 1998-04-25 18:25 More fp bug in egcs H.J. Lu 1998-04-27 21:29 ` Jim Wilson @ 1998-04-30 20:03 ` Jim Wilson 1998-05-02 18:56 ` H.J. Lu ` (2 more replies) 1 sibling, 3 replies; 22+ messages in thread From: Jim Wilson @ 1998-04-30 20:03 UTC (permalink / raw) To: H.J. Lu; +Cc: wilson, law, scox, crux, egcs I believe this is another bug in the same i386 code as my last patch. The problem is that the only FP->DImode converstion instruction pops the FP stack. Normally we have both popping and non-popping versions of instructions. The x86 code handles this by aborting if we need to emit the non-existent non-popping instruction. However, this can't work all of the time, because it assumes the optimizer always generates optimal code. That isn't safe. And it is obviously not safe if we are compiling without optimization. In order to fix this, we need to emulate the missing instruction if gcc needs to emit it. The following patch does this. If there is a better way to do this, then let me know. Thu Apr 30 19:28:16 1998 Jim Wilson <wilson@cygnus.com> * i386.c (output_fix_trunc): Add code to emulate non-popping DImode case. *** i386.c Sun Feb 15 11:54:11 1998 --- /home/wilson/tmp/i386.c Thu Apr 30 19:26:54 1998 *************** output_fix_trunc (insn, operands) *** 3731,3738 **** int stack_top_dies = find_regno_note (insn, REG_DEAD, FIRST_STACK_REG) != 0; rtx xops[2]; ! if (! STACK_TOP_P (operands[1]) || ! (GET_MODE (operands[0]) == DImode && ! stack_top_dies)) abort (); xops[0] = GEN_INT (12); --- 3731,3737 ---- int stack_top_dies = find_regno_note (insn, REG_DEAD, FIRST_STACK_REG) != 0; rtx xops[2]; ! if (! STACK_TOP_P (operands[1])) abort (); xops[0] = GEN_INT (12); *************** output_fix_trunc (insn, operands) *** 3750,3755 **** --- 3749,3765 ---- { if (stack_top_dies) output_asm_insn (AS1 (fistp%z0,%0), operands); + else if (GET_MODE (operands[0]) == DImode && ! stack_top_dies) + { + /* There is no DImode version of this without a stack pop, so + we must emulate it. It doesn't matter much what the second + instruction is, because the value being pushed on the FP stack + is not used except for the following stack popping store. + This case can only happen without optimization, so it doesn't + matter that it is inefficient. */ + output_asm_insn (AS1 (fistp%z0,%0), operands); + output_asm_insn (AS1 (fild%z0,%0), operands); + } else output_asm_insn (AS1 (fist%z0,%0), operands); } ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: More fp bug in egcs 1998-04-30 20:03 ` Jim Wilson @ 1998-05-02 18:56 ` H.J. Lu 1998-05-03 20:10 ` Jim Wilson 1998-05-05 0:35 ` Jeffrey A Law 1998-05-05 5:03 ` Jeffrey A Law 1998-05-06 17:12 ` A patch for PPro H.J. Lu 2 siblings, 2 replies; 22+ messages in thread From: H.J. Lu @ 1998-05-02 18:56 UTC (permalink / raw) To: Jim Wilson; +Cc: hjl, wilson, law, scox, crux, egcs > > I believe this is another bug in the same i386 code as my last patch. > > The problem is that the only FP->DImode converstion instruction pops the > FP stack. Normally we have both popping and non-popping versions of > instructions. The x86 code handles this by aborting if we need to emit > the non-existent non-popping instruction. However, this can't work all > of the time, because it assumes the optimizer always generates optimal > code. That isn't safe. And it is obviously not safe if we are compiling > without optimization. > > In order to fix this, we need to emulate the missing instruction if gcc > needs to emit it. The following patch does this. If there is a better way > to do this, then let me know. > > Thu Apr 30 19:28:16 1998 Jim Wilson <wilson@cygnus.com> > > * i386.c (output_fix_trunc): Add code to emulate non-popping DImode > case. > > *** i386.c Sun Feb 15 11:54:11 1998 > --- /home/wilson/tmp/i386.c Thu Apr 30 19:26:54 1998 > *************** output_fix_trunc (insn, operands) > *** 3731,3738 **** > int stack_top_dies = find_regno_note (insn, REG_DEAD, FIRST_STACK_REG) != 0; > rtx xops[2]; > > ! if (! STACK_TOP_P (operands[1]) || > ! (GET_MODE (operands[0]) == DImode && ! stack_top_dies)) > abort (); > > xops[0] = GEN_INT (12); > --- 3731,3737 ---- > int stack_top_dies = find_regno_note (insn, REG_DEAD, FIRST_STACK_REG) != 0; > rtx xops[2]; > > ! if (! STACK_TOP_P (operands[1])) > abort (); > > xops[0] = GEN_INT (12); > *************** output_fix_trunc (insn, operands) > *** 3750,3755 **** > --- 3749,3765 ---- > { > if (stack_top_dies) > output_asm_insn (AS1 (fistp%z0,%0), operands); > + else if (GET_MODE (operands[0]) == DImode && ! stack_top_dies) > + { > + /* There is no DImode version of this without a stack pop, so > + we must emulate it. It doesn't matter much what the second > + instruction is, because the value being pushed on the FP stack > + is not used except for the following stack popping store. > + This case can only happen without optimization, so it doesn't > + matter that it is inefficient. */ > + output_asm_insn (AS1 (fistp%z0,%0), operands); > + output_asm_insn (AS1 (fild%z0,%0), operands); > + } > else > output_asm_insn (AS1 (fist%z0,%0), operands); > } > Here is the trimmed down test case. I am not sure if your patch is correct. If you take look at the stack RTL dump, you will see SF 1 in (define_insn "" [(set (match_operand:DI 0 "nonimmediate_operand" "=rm") (fix:DI (fix:SF (match_operand:SF 1 "register_operand" "+f")))) (clobber (match_dup 1)) (clobber (match_operand:SI 2 "memory_operand" "m")) (clobber (match_operand:DI 3 "memory_operand" "m")) (clobber (match_scratch:SI 4 "=&q"))] "TARGET_80387" "* return output_fix_trunc (insn, operands);") is used as the input for the next insn: ;; Insn is not within a basic block (insn:QI 104 269 272 (parallel[ (set (mem:DI (plus:SI (reg:SI 6 %ebp) (const_int -144))) (fix:DI (fix:SF (reg:SF 8 %st(0))))) (clobber (reg:SF 8 %st(0))) (clobber (mem:SI (plus:SI (reg:SI 6 %ebp) (const_int -4)))) (clobber (mem:DI (plus:SI (reg:SI 6 %ebp) (const_int -12)))) (clobber (reg:SI 1 %edx)) ] ) 117 {fix_truncxfsi2-1} (nil) (nil)) ;; Insn is not within a basic block (insn:QI 272 104 275 (set (mem:SF (plus:SI (reg:SI 6 %ebp) (const_int -148))) (reg:SF 8 %st(0))) -1 (nil) (expr_list:REG_DEAD (reg:DF 8 %st(0)) (nil))) I don't know if it is correct. Did gcc know %st(0) was not the same %st(0) before? Thanks. -- H.J. Lu (hjl@gnu.org) --- typedef struct _geom_elem { double coeffs[6]; } pGeomDefRec, *pGeomDefPtr; typedef struct _mpgeombanddef { int yOut; int in_width; } mpGeometryBandRec, *mpGeometryBandPtr; typedef void *pointer; typedef unsigned char CARD8; typedef CARD8 BytePixel; void BiGL_B (OUTP,srcimg,width,sline,pedpvt,pvtband) pointer OUTP; pointer *srcimg; register int width; int sline; pGeomDefPtr pedpvt; mpGeometryBandPtr pvtband; { register float s, t, st; register int isrcline,isrcpix; register int srcwidth = pvtband->in_width - 1; register BytePixel val; register BytePixel *ptrIn, *ptrJn; register double a = pedpvt->coeffs[0]; register double c = pedpvt->coeffs[2]; register double srcpix = a * ((double)(0.0000)) + pedpvt->coeffs[1] * (pvtband->yOut + ((double)(0.0000)) ) + pedpvt->coeffs[4]; register double srcline = c * ((double)(0.0000)) + pedpvt->coeffs[3] * (pvtband->yOut + ((double)(0.0000)) ) + pedpvt->coeffs[5]; if ( (isrcpix >= 0) && (isrcpix < srcwidth) ) val = ptrIn[isrcpix] * ((float)1. - s - t + st) + ptrIn[isrcpix+1] * (s - st) + ptrJn[isrcpix] * (t - st) + ptrJn[isrcpix+1] * (st) + (float)0.5 ; } ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: More fp bug in egcs 1998-05-02 18:56 ` H.J. Lu @ 1998-05-03 20:10 ` Jim Wilson 1998-05-05 0:35 ` Jeffrey A Law 1 sibling, 0 replies; 22+ messages in thread From: Jim Wilson @ 1998-05-03 20:10 UTC (permalink / raw) To: H.J. Lu; +Cc: law, scox, crux, egcs Here is the trimmed down test case. I am not sure if your patch is correct. If you take look at the stack RTL dump, you will see SF 1 in is used as the input for the next insn: It is supposed to be used by the next instruction. This is the output reload that copies the input/output value to the stack slot where it lives. However, since this value is dead, it will never be read from the stack slot, and hence it doesn't matter what this value is. The only problem here is that the code is inefficient, but since this case can only happen when not optimized, this is not a problem. I don't know if it is correct. Did gcc know %st(0) was not the same %st(0) before? Yes, gcc knows that %st(0) was clobbered. This is why the following instruction is storing the clobbered value back to the stack slot where it lives. When considering how to fix this bug, it is important to separate what the RTL means from what the actual x86 FP instructions are. My patch fixes the problem by synthesizing a missing instruction. This is a perfectly valid thing to do. This is no different than using two 4-byte loads to make a 8-byte load. It is just a bit harder to understand because the FP stack is involved. Jim ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: More fp bug in egcs 1998-05-02 18:56 ` H.J. Lu 1998-05-03 20:10 ` Jim Wilson @ 1998-05-05 0:35 ` Jeffrey A Law 1998-05-05 19:14 ` H.J. Lu 1 sibling, 1 reply; 22+ messages in thread From: Jeffrey A Law @ 1998-05-05 0:35 UTC (permalink / raw) To: H.J. Lu; +Cc: Jim Wilson, scox, crux, egcs In message < m0yVnEy-000268C@ocean.lucon.org >you write: > Here is the trimmed down test case. I am not sure if your patch is > correct. If you take look at the stack RTL dump, you will see SF 1 in [ ... ] BTW, I added the testcase to the testsuite. jeff ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: More fp bug in egcs 1998-05-05 0:35 ` Jeffrey A Law @ 1998-05-05 19:14 ` H.J. Lu 1998-05-06 11:49 ` Jim Wilson 0 siblings, 1 reply; 22+ messages in thread From: H.J. Lu @ 1998-05-05 19:14 UTC (permalink / raw) To: law; +Cc: hjl, wilson, scox, crux, egcs > > > In message < m0yVnEy-000268C@ocean.lucon.org >you write: > > Here is the trimmed down test case. I am not sure if your patch is > > correct. If you take look at the stack RTL dump, you will see SF 1 in > [ ... ] > BTW, I added the testcase to the testsuite. > > jeff > Here is the stack RTL dump again. As you can see (clobber (reg:SF 14 %st(6))) should be (clobber (reg:SF 8 %st(0))) Should I worry about it? I think the bug is in move_for_stack_reg () in reg-stack.c: else if (STACK_REG_P (src)) { /* Save from a stack reg to MEM, or possibly integer reg. Since only top of stack may be saved, emit an exchange first if needs be. */ emit_swap_insn (insn, regstack, src); note = find_regno_note (insn, REG_DEAD, REGNO (src)); if (note) { replace_reg (&XEXP (note, 0), FIRST_STACK_REG); regstack->top--; CLEAR_HARD_REG_BIT (regstack->reg_set, REGNO (src)); } else if (GET_MODE (src) == XFmode && regstack->top < REG_STACK_SIZE - 1) { /* A 387 cannot write an XFmode value to a MEM without clobbering the source reg. The output code can handle this by reading back the value from the MEM. But it is more efficient to use a temp register if one is available. Push the source value here if the register stack is not full, and then write the value to memory via a pop. */ rtx push_rtx, push_insn; rtx top_stack_reg = FP_MODE_REG (FIRST_STACK_REG, XFmode); push_rtx = gen_movxf (top_stack_reg, top_stack_reg); push_insn = emit_insn_before (push_rtx, insn); PUT_MODE (push_insn, QImode); REG_NOTES (insn) = gen_rtx (EXPR_LIST, REG_DEAD, top_stack_reg, REG_NOTES (insn)); } replace_reg (psrc, FIRST_STACK_REG); } The problem is not all notes are updated. -- H.J. Lu (hjl@gnu.org) ---- ;; Insn is not within a basic block (insn:QI 67 163 166 (parallel[ (set (reg:DI 2 %ecx) (fix:DI (fix:SF (reg:SF 8 %st(0))))) (clobber (reg:SF 14 %st(6))) (clobber (mem:SI (plus:SI (reg:SI 6 %ebp) (const_int -20)))) (clobber (mem:DI (plus:SI (reg:SI 6 %ebp) (const_int -28)))) (clobber (reg:SI 1 %edx)) ] ) 120 {fix_truncxfsi2-1} (nil) (nil)) ;; Insn is not within a basic block (insn:QI 166 67 68 (set (mem:SF (plus:SI (reg:SI 6 %ebp) (const_int -76))) (reg:SF 8 %st(0))) -1 (nil) (expr_list:REG_DEAD (reg:DF 8 %st(0)) (nil))) ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: More fp bug in egcs 1998-05-05 19:14 ` H.J. Lu @ 1998-05-06 11:49 ` Jim Wilson 0 siblings, 0 replies; 22+ messages in thread From: Jim Wilson @ 1998-05-06 11:49 UTC (permalink / raw) To: H.J. Lu; +Cc: law, scox, crux, egcs Here is the stack RTL dump again. As you can see (clobber (reg:SF 14 %st(6))) should be (clobber (reg:SF 8 %st(0))) Should I worry about it? I think the bug is in move_for_stack_reg () in reg-stack.c: There is no need to worry about it for the egcs-1.0.3 release or the egcs-1.1.x release. It would be nice to get this fixed in the development sources in the long term, but for now it is perfectly harmless. There is a general problem that reg-stack does not perform register substitutions in clobbers. The fix will probably be involved, and would not be safe for any pending release. Jim ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: More fp bug in egcs 1998-04-30 20:03 ` Jim Wilson 1998-05-02 18:56 ` H.J. Lu @ 1998-05-05 5:03 ` Jeffrey A Law 1998-05-06 17:12 ` A patch for PPro H.J. Lu 2 siblings, 0 replies; 22+ messages in thread From: Jeffrey A Law @ 1998-05-05 5:03 UTC (permalink / raw) To: Jim Wilson; +Cc: H.J. Lu, scox, crux, egcs In message <199805010234.TAA02560@ada.cygnus.com.cygnus.com>you write: > I believe this is another bug in the same i386 code as my last patch. > > The problem is that the only FP->DImode converstion instruction pops the > FP stack. Normally we have both popping and non-popping versions of > instructions. The x86 code handles this by aborting if we need to emit > the non-existent non-popping instruction. However, this can't work all > of the time, because it assumes the optimizer always generates optimal > code. That isn't safe. And it is obviously not safe if we are compiling > without optimization. > > In order to fix this, we need to emulate the missing instruction if gcc > needs to emit it. The following patch does this. If there is a better wa > y > to do this, then let me know. > > Thu Apr 30 19:28:16 1998 Jim Wilson <wilson@cygnus.com> > > * i386.c (output_fix_trunc): Add code to emulate non-popping DImode > case. I went ahead and checked this into the release branch and the mainline sources. jeff ^ permalink raw reply [flat|nested] 22+ messages in thread
* A patch for PPro 1998-04-30 20:03 ` Jim Wilson 1998-05-02 18:56 ` H.J. Lu 1998-05-05 5:03 ` Jeffrey A Law @ 1998-05-06 17:12 ` H.J. Lu 1998-05-06 18:14 ` Jeffrey A Law 1998-05-07 15:31 ` Jim Wilson 2 siblings, 2 replies; 22+ messages in thread From: H.J. Lu @ 1998-05-06 17:12 UTC (permalink / raw) To: Jim Wilson; +Cc: law, scox, crux, egcs Hi, The current egcs in CVS disables fp conditional move for PPro. The problem is mark_constants has to search inside a CONST_DOUBLE when it is enabled. I understand it is not safe to do so. I am enclosing a patch which will search inside a CONST_DOUBLE only for those PPro fp conditional moves. It won't affect any other CPUs, even other x86 instructions. H.J. --- Wed May 6 07:35:26 1998 H.J. Lu (hjl@gnu.org) * config/i386/i386.md (movsicc_1, movhicc_1): Use output_int_conditional_move (). (movxfcc_1, movdfcc_1, movxfcc_1): Fix typos, use output_fp_conditional_move () and allow constants for reload. * config/i386/i386.h (IS_SEARCH_CONST_DOUBLE_OK): New, defined as is_search_const_double_ok (insn). * config/i386/i386.c (output_fp_conditional_move): New function to hanld fp conditional move. (output_int_conditional_move): New function to handle integer conditional move. (is_search_const_double_ok): New function to check if it is ok search inside a CONST_DOUBLE used by INSN. * varasm.c (IS_SEARCH_CONST_DOUBLE_OK): New. True if it is ok to search inside a CONST_DOUBLE used by INSN. Default to false. (mark_constant_pool): Call mark_constants with a new arg, IS_SEARCH_CONST_DOUBLE_OK (insn). (mark_constants): Add a new arg to indicate if the instruction is ok to search inside a CONST_DOUBLE. Look inside CONST_DOUBLE if it is in memory and it is ok to do so. * print-rtl.c (get_insn_name): New. Return the insn name string if there is one, otherwise NULL. * rtl.h (get_insn_name): New declaration. Index: config/i386/i386.md =================================================================== RCS file: /home/work/cvs/gnu/egcs/gcc/config/i386/i386.md,v retrieving revision 1.1.1.14 diff -u -r1.1.1.14 i386.md --- config/i386/i386.md 1998/04/21 15:42:54 1.1.1.14 +++ config/i386/i386.md 1998/04/21 15:48:12 @@ -7259,58 +7260,7 @@ (match_operand:SI 3 "general_operand" "0,rm,rm,g"))) (clobber (match_scratch:SI 4 "X,X,X,=&r"))] "TARGET_CMOVE" - "* -{ - if (which_alternative == 0) - { - /* r <- cond ? arg : r */ - output_asm_insn (AS2 (cmov%C1,%2,%0), operands); - } - else if (which_alternative == 1) - { - /* r <- cond ? r : arg */ - output_asm_insn (AS2 (cmov%c1,%3,%0), operands); - } - else if (which_alternative == 2) - { - /* r <- cond ? arg1 : arg2 */ - output_asm_insn (AS2 (cmov%C1,%2,%0), operands); - output_asm_insn (AS2 (cmov%c1,%3,%0), operands); - } - else if (which_alternative == 3) - { - /* r <- cond ? arg1 : arg2 */ - rtx xops[3]; - - xops[0] = gen_label_rtx (); - xops[1] = gen_label_rtx (); - xops[2] = operands[1]; - - output_asm_insn (\"j%c2 %l0\", xops); - if (! rtx_equal_p (operands[0], operands[2])) - if (GET_CODE (operands[0]) == MEM && GET_CODE (operands[2]) == MEM) - { - output_asm_insn (AS2 (mov%z2,%2,%4), operands); - output_asm_insn (AS2 (mov%z2,%4,%0), operands); - } - else - output_asm_insn (AS2 (mov%z0,%2,%0), operands); - output_asm_insn (\"jmp %l1\", xops); - ASM_OUTPUT_INTERNAL_LABEL (asm_out_file, \"L\", CODE_LABEL_NUMBER (xops[0])); - if (! rtx_equal_p (operands[0], operands[3])) - { - if (GET_CODE (operands[0]) == MEM && GET_CODE (operands[3]) == MEM) - { - output_asm_insn (AS2 (mov%z3,%3,%4), operands); - output_asm_insn (AS2 (mov%z3,%4,%0), operands); - } - else - output_asm_insn (AS2 (mov%z0,%3,%0), operands); - } - ASM_OUTPUT_INTERNAL_LABEL (asm_out_file, \"L\", CODE_LABEL_NUMBER (xops[1])); - } - RET; -}") + "* return output_int_conditional_move (which_alternative, operands);") (define_insn "movhicc_1" [(set (match_operand:HI 0 "nonimmediate_operand" "=r,r,&r,rm") @@ -7320,69 +7270,26 @@ (match_operand:HI 3 "general_operand" "0,rm,rm,g"))) (clobber (match_scratch:SI 4 "X,X,X,=&r"))] "TARGET_CMOVE" - "* -{ - if (which_alternative == 0) - { - /* r <- cond ? arg : r */ - output_asm_insn (AS2 (cmov%C1,%2,%0), operands); - } - else if (which_alternative == 1) - { - /* r <- cond ? r : arg */ - output_asm_insn (AS2 (cmov%c1,%3,%0), operands); - } - else if (which_alternative == 2) - { - /* r <- cond ? arg1 : arg2 */ - output_asm_insn (AS2 (cmov%C1,%2,%0), operands); - output_asm_insn (AS2 (cmov%c1,%3,%0), operands); - } - else if (which_alternative == 3) - { - /* r <- cond ? arg1 : arg2 */ - rtx xops[3]; - - xops[0] = gen_label_rtx (); - xops[1] = gen_label_rtx (); - xops[2] = operands[1]; - - output_asm_insn (\"j%c2 %l0\", xops); - if (! rtx_equal_p (operands[0], operands[2])) - if (GET_CODE (operands[0]) == MEM && GET_CODE (operands[2]) == MEM) - { - output_asm_insn (AS2 (mov%z2,%2,%4), operands); - output_asm_insn (AS2 (mov%z2,%4,%0), operands); - } - else - output_asm_insn (AS2 (mov%z0,%2,%0), operands); - output_asm_insn (\"jmp %l1\", xops); - ASM_OUTPUT_INTERNAL_LABEL (asm_out_file, \"L\", CODE_LABEL_NUMBER (xops[0])); - if (! rtx_equal_p (operands[0], operands[3])) - { - if (GET_CODE (operands[0]) == MEM && GET_CODE (operands[3]) == MEM) - { - output_asm_insn (AS2 (mov%z3,%3,%4), operands); - output_asm_insn (AS2 (mov%z3,%4,%0), operands); - } - else - output_asm_insn (AS2 (mov%z0,%3,%0), operands); - } - ASM_OUTPUT_INTERNAL_LABEL (asm_out_file, \"L\", CODE_LABEL_NUMBER (xops[1])); - } - RET; -}") + "* return output_int_conditional_move (which_alternative, operands);") + ;; We need to disable the FP forms of these since they do not support ;; memory as written, but no input reloads are permitted for insns ;; that use cc0. Also, movxfcc is not present. +;; ??? We should add support for constant values to each of the mov*fcc +;; patterns. Apparently only values accepted by standard_80387_constant_p +;; are valid. We need to create a new predicate that accepts all +;; nonimmediate_operand values plus those constants accepts by s_8_c_p. +;; We need to add 'G' to the constraints where appropriate. We need to +;; add code to the alternative 3 cases that knows how to load such constants. + (define_expand "movsfcc" [(match_dup 4) - (set (match_operand 0 "register_operand" "") + (set (match_operand 0 "register_operand" "t") (if_then_else:SF (match_operand 1 "comparison_operator" "") - (match_operand:SF 2 "register_operand" "") - (match_operand:SF 3 "register_operand" "")))] - "0 && TARGET_CMOVE" + (match_operand:SF 2 "nonimmediate_operand" "") + (match_operand:SF 3 "nonimmediate_operand" "")))] + "TARGET_CMOVE" " { operands[4] = i386_compare_gen (i386_compare_op0, i386_compare_op1); @@ -7392,9 +7299,9 @@ [(match_dup 4) (set (match_operand 0 "register_operand" "t") (if_then_else:DF (match_operand 1 "comparison_operator" "") - (match_operand:DF 2 "register_operand" "") - (match_operand:DF 3 "register_operand" "")))] - "0 && TARGET_CMOVE" + (match_operand:DF 2 "nonimmediate_operand" "") + (match_operand:DF 3 "nonimmediate_operand" "")))] + "TARGET_CMOVE" " { operands[4] = i386_compare_gen (i386_compare_op0, i386_compare_op1); @@ -7404,75 +7311,40 @@ [(match_dup 4) (set (match_operand 0 "register_operand" "") (if_then_else:XF (match_operand 1 "comparison_operator" "") - (match_operand:XF 2 "register_operand" "") - (match_operand:XF 3 "register_operand" "")))] - "0 && TARGET_CMOVE" + (match_operand:XF 2 "nonimmediate_operand" "") + (match_operand:XF 3 "nonimmediate_operand" "")))] + "TARGET_CMOVE" " { operands[4] = i386_compare_gen (i386_compare_op0, i386_compare_op1); }") (define_insn "movsfcc_1" - [(set (match_operand:SF 0 "general_operand" "=f,f,&f") + [(set (match_operand:SF 0 "register_operand" "=f,=f,=f,=f") (if_then_else:SF (match_operator 1 "comparison_operator" [(cc0) (const_int 0)]) - (match_operand:SF 2 "register_operand" "0,f,f") - (match_operand:SF 3 "register_operand" "f,0,f")))] + (match_operand:SF 2 "nonimmediate_operand" "0,f,f,fFm") + (match_operand:SF 3 "nonimmediate_operand" "f,0,f,fFm")))] "TARGET_CMOVE" - "* -{ - switch (which_alternative) - { - case 0: - /* r <- cond ? arg : r */ - output_asm_insn (AS2 (fcmov%f1,%3,%0), operands); - break; - - case 1: - /* r <- cond ? r : arg */ - output_asm_insn (AS2 (fcmov%F1,%2,%0), operands); - break; - - case 2: - /* r <- cond ? r : arg */ - output_asm_insn (AS2 (fcmov%F1,%2,%0), operands); - output_asm_insn (AS2 (fcmov%f1,%3,%0), operands); - break; - } - - RET; -}") + "* return output_fp_conditional_move (which_alternative, operands);") (define_insn "movdfcc_1" - [(set (match_operand:DF 0 "general_operand" "=f,f,&f") + [(set (match_operand:DF 0 "register_operand" "=f,=f,=f,=f") (if_then_else:DF (match_operator 1 "comparison_operator" [(cc0) (const_int 0)]) - (match_operand:DF 2 "register_operand" "0,f,f") - (match_operand:DF 3 "register_operand" "f,0,f")))] + (match_operand:DF 2 "nonimmediate_operand" "0,f,f,fFm") + (match_operand:DF 3 "nonimmediate_operand" "f,0,f,fFm")))] "TARGET_CMOVE" - "* -{ - switch (which_alternative) - { - case 0: - /* r <- cond ? arg : r */ - output_asm_insn (AS2 (fcmov%f1,%3,%0), operands); - break; - - case 1: - /* r <- cond ? r : arg */ - output_asm_insn (AS2 (fcmov%F1,%2,%0), operands); - break; + "* return output_fp_conditional_move (which_alternative, operands);") - case 2: - /* r <- cond ? r : arg */ - output_asm_insn (AS2 (fcmov%F1,%2,%0), operands); - output_asm_insn (AS2 (fcmov%f1,%3,%0), operands); - break; - } - - RET; -}") +(define_insn "movxfcc_1" + [(set (match_operand:XF 0 "register_operand" "=f,=f,=f,=f") + (if_then_else:XF (match_operator 1 "comparison_operator" + [(cc0) (const_int 0)]) + (match_operand:XF 2 "nonimmediate_operand" "0,f,f,fFm") + (match_operand:XF 3 "nonimmediate_operand" "f,0,f,fFm")))] + "TARGET_CMOVE" + "* return output_fp_conditional_move (which_alternative, operands);") (define_insn "strlensi_unroll" [(set (match_operand:SI 0 "register_operand" "=&r,&r") Index: config/i386/i386.h =================================================================== RCS file: /home/work/cvs/gnu/egcs/gcc/config/i386/i386.h,v retrieving revision 1.1.1.15 diff -u -r1.1.1.15 i386.h --- config/i386/i386.h 1998/04/21 15:42:51 1.1.1.15 +++ config/i386/i386.h 1998/05/06 18:08:55 @@ -817,6 +817,10 @@ #define CONST_DOUBLE_OK_FOR_LETTER_P(VALUE, C) \ ((C) == 'G' ? standard_80387_constant_p (VALUE) : 0) +/* Is ok to search inside a CONST_DOUBLE? */ +#define IS_SEARCH_CONST_DOUBLE_OK(insn) \ + is_search_const_double_ok (insn) + /* Place additional restrictions on the register class to use when it is necessary to be able to hold a value of mode MODE in a reload register for which class CLASS would ordinarily be used. */ Index: config/i386/i386.c =================================================================== RCS file: /home/work/cvs/gnu/egcs/gcc/config/i386/i386.c,v retrieving revision 1.1.1.13 diff -u -r1.1.1.13 i386.c --- config/i386/i386.c 1998/05/05 22:04:03 1.1.1.13 +++ config/i386/i386.c 1998/05/06 18:07:22 @@ -5107,4 +5116,158 @@ ASM_OUTPUT_INTERNAL_LABEL (asm_out_file, "L", CODE_LABEL_NUMBER (xops[12])); return ""; +} + +char * +output_fp_conditional_move (which_alternative, operands) + int which_alternative; + rtx operands[]; +{ + if (which_alternative == 0) + { + /* r <- cond ? arg : r */ + output_asm_insn (AS2 (fcmov%f1,%3,%0), operands); + } + else if (which_alternative == 1) + { + /* r <- cond ? r : arg */ + output_asm_insn (AS2 (fcmov%F1,%2,%0), operands); + } + else if (which_alternative == 2) + { + /* r <- cond ? r : arg */ + output_asm_insn (AS2 (fcmov%F1,%2,%0), operands); + output_asm_insn (AS2 (fcmov%f1,%3,%0), operands); + } + else if (which_alternative == 3) + { + /* r <- cond ? arg1 : arg2 */ + rtx xops[3]; + + xops[0] = gen_label_rtx (); + xops[1] = gen_label_rtx (); + xops[2] = operands[1]; + + output_asm_insn ("j%f2 %l0", xops); + if (STACK_REG_P (operands[2]) || GET_CODE (operands[2]) == MEM) + output_asm_insn (AS1 (fld%z2,%y2), operands); + else + { + int conval = standard_80387_constant_p (operands[2]); + + switch (conval) + { + case 1: + fprintf (asm_out_file, "\tfldz\n"); + break; + case 2: + fprintf (asm_out_file, "\tfld1\n"); + break; + default: + operands[2] = CONST_DOUBLE_MEM (operands[2]); + if (GET_CODE (operands[2]) != MEM) + abort (); + output_asm_insn (AS1 (fld%z2,%y2), operands); + break; + } + } + output_asm_insn ("jmp %l1", xops); + ASM_OUTPUT_INTERNAL_LABEL (asm_out_file, "L", CODE_LABEL_NUMBER (xops[0])); + if (STACK_REG_P (operands[3]) || GET_CODE (operands[3]) == MEM) + output_asm_insn (AS1 (fld%z3,%y3), operands); + else + { + int conval = standard_80387_constant_p (operands[2]); + + switch (conval) + { + case 1: + fprintf (asm_out_file, "\tfldz\n"); + break; + case 2: + fprintf (asm_out_file, "\tfld1\n"); + break; + default: + operands[3] = CONST_DOUBLE_MEM (operands[3]); + if (GET_CODE (operands[3]) != MEM) + abort (); + output_asm_insn (AS1 (fld%z3,%y3), operands); + break; + } + } + ASM_OUTPUT_INTERNAL_LABEL (asm_out_file, "L", CODE_LABEL_NUMBER (xops[1])); + } + + return ""; +} + +char * +output_int_conditional_move (which_alternative, operands) + int which_alternative; + rtx operands[]; +{ + if (which_alternative == 0) + { + /* r <- cond ? arg : r */ + output_asm_insn (AS2 (cmov%C1,%2,%0), operands); + } + else if (which_alternative == 1) + { + /* r <- cond ? r : arg */ + output_asm_insn (AS2 (cmov%c1,%3,%0), operands); + } + else if (which_alternative == 2) + { + /* r <- cond ? arg1 : arg2 */ + output_asm_insn (AS2 (cmov%C1,%2,%0), operands); + output_asm_insn (AS2 (cmov%c1,%3,%0), operands); + } + else if (which_alternative == 3) + { + /* r <- cond ? arg1 : arg2 */ + rtx xops[3]; + + xops[0] = gen_label_rtx (); + xops[1] = gen_label_rtx (); + xops[2] = operands[1]; + + output_asm_insn ("j%c2 %l0", xops); + if (! rtx_equal_p (operands[0], operands[2])) + { + if (GET_CODE (operands[0]) == MEM + && GET_CODE (operands[2]) == MEM) + { + output_asm_insn (AS2 (mov%z2,%2,%4), operands); + output_asm_insn (AS2 (mov%z2,%4,%0), operands); + } + else + output_asm_insn (AS2 (mov%z0,%2,%0), operands); + } + output_asm_insn ("jmp %l1", xops); + ASM_OUTPUT_INTERNAL_LABEL (asm_out_file, "L", CODE_LABEL_NUMBER (xops[0])); + if (! rtx_equal_p (operands[0], operands[3])) + { + if (GET_CODE (operands[0]) == MEM + && GET_CODE (operands[3]) == MEM) + { + output_asm_insn (AS2 (mov%z3,%3,%4), operands); + output_asm_insn (AS2 (mov%z3,%4,%0), operands); + } + else + output_asm_insn (AS2 (mov%z0,%3,%0), operands); + } + ASM_OUTPUT_INTERNAL_LABEL (asm_out_file, "L", CODE_LABEL_NUMBER (xops[1])); + } + + return ""; +} + +int +is_search_const_double_ok (insn) + rtx insn; +{ + const char * name = get_insn_name (insn); + return name && (strcmp (name, "movsfcc_1") == 0 + || strcmp (name, "movdfcc_1") == 0 + || strcmp (name, "movxfcc_1") == 0); } Index: varasm.c =================================================================== RCS file: /home/work/cvs/gnu/egcs/gcc/varasm.c,v retrieving revision 1.1.1.15 diff -u -r1.1.1.15 varasm.c --- varasm.c 1998/04/17 15:01:32 1.1.1.15 +++ varasm.c 1998/05/06 18:09:07 @@ -155,7 +155,7 @@ rtx)); static struct pool_constant *find_pool_constant PROTO((rtx)); static void mark_constant_pool PROTO((void)); -static void mark_constants PROTO((rtx)); +static void mark_constants PROTO((rtx, int)); static int output_addressed_constants PROTO((tree)); static void output_after_function_constants PROTO((void)); static void output_constructor PROTO((tree, int)); @@ -3651,6 +3652,11 @@ first_pool = last_pool = 0; } +/* Is ok to search inside a CONST_DOUBLE? Default to NO. */ +#ifndef IS_SEARCH_CONST_DOUBLE_OK +#define IS_SEARCH_CONST_DOUBLE_OK(insn) (0) +#endif + /* Look through the instructions for this function, and mark all the entries in the constant pool which are actually being used. */ @@ -3668,18 +3674,19 @@ for (insn = get_insns (); insn; insn = NEXT_INSN (insn)) if (GET_RTX_CLASS (GET_CODE (insn)) == 'i') - mark_constants (PATTERN (insn)); + mark_constants (PATTERN (insn), IS_SEARCH_CONST_DOUBLE_OK (insn)); for (insn = current_function_epilogue_delay_list; insn; insn = XEXP (insn, 1)) if (GET_RTX_CLASS (GET_CODE (insn)) == 'i') - mark_constants (PATTERN (insn)); + mark_constants (PATTERN (insn), IS_SEARCH_CONST_DOUBLE_OK (insn)); } static void -mark_constants (x) +mark_constants (x, fp_mem_ok) register rtx x; + int fp_mem_ok; { register int i; register char *format_ptr; @@ -3697,8 +3704,16 @@ a MEM, but does not constitute a use of that MEM. This is particularly important inside a nested function, because CONST_DOUBLE_MEM may be a reference to a MEM in the parent's constant pool. See the comment - in force_const_mem. */ - else if (GET_CODE (x) == CONST_DOUBLE) + in force_const_mem. + + For a special case on x86 with FP conditional move, reload may + generate patterns containing CONST_DOUBLE such that the x86 + backend will generate load memory instuctions behind the back. To + cover this, we check if the instruction in which the CONST_DOUBLE + is used is a SET. If the instruction is a SET and CONST_DOUBLE is + in memory, we will look inside the CONST_DOUBLE. */ + else if (GET_CODE (x) == CONST_DOUBLE + && (!fp_mem_ok || GET_CODE (CONST_DOUBLE_MEM (x)) != MEM)) return; /* Insns may appear inside a SEQUENCE. Only check the patterns of @@ -3706,7 +3721,7 @@ a constant just because it happens to appear in a REG_EQUIV note. */ if (GET_RTX_CLASS (GET_CODE (x)) == 'i') { - mark_constants (PATTERN (x)); + mark_constants (PATTERN (x), fp_mem_ok); return; } @@ -3717,7 +3732,7 @@ switch (*format_ptr++) { case 'e': - mark_constants (XEXP (x, i)); + mark_constants (XEXP (x, i), fp_mem_ok); break; case 'E': @@ -3726,7 +3741,7 @@ register int j; for (j = 0; j < XVECLEN (x, i); j++) - mark_constants (XVECEXP (x, i, j)); + mark_constants (XVECEXP (x, i, j), fp_mem_ok); } break; Index: rtl.h =================================================================== RCS file: /home/work/cvs/gnu/egcs/gcc/rtl.h,v retrieving revision 1.1.1.17 diff -u -r1.1.1.17 rtl.h --- rtl.h 1998/04/28 22:45:33 1.1.1.17 +++ rtl.h 1998/05/06 17:57:54 @@ -1207,6 +1196,7 @@ extern void print_rtl PROTO ((FILE *, rtx)); extern void print_inline_rtx PROTO ((FILE *, rtx, int)); #endif +extern const char *get_insn_name PROTO ((rtx)); /* In loop.c */ extern void init_loop PROTO ((void)); Index: print-rtl.c =================================================================== RCS file: /home/work/cvs/gnu/egcs/gcc/print-rtl.c,v retrieving revision 1.1.1.5 diff -u -r1.1.1.5 print-rtl.c --- print-rtl.c 1998/03/21 01:48:28 1.1.1.5 +++ print-rtl.c 1998/05/06 19:08:37 @@ -52,6 +52,8 @@ extern char **insn_name_ptr; +static void print_rtx PROTO ((rtx)); + /* Print IN_RTX onto OUTFILE. This is the recursive part of printing. */ static void @@ -362,4 +364,24 @@ sawclose = 0; print_rtx (x); putc ('\n', outf); +} + +const char * +get_insn_name (in_rtx) + register rtx in_rtx; +{ + if (GET_RTX_CLASS (GET_CODE (in_rtx)) == 'i' && insn_name_ptr) + { + register int i; + register char *format_ptr; + + format_ptr = GET_RTX_FORMAT (GET_CODE (in_rtx)); + for (i = 0; i < GET_RTX_LENGTH (GET_CODE (in_rtx)); i++) + if (*format_ptr++ == 'i' + && &INSN_CODE (in_rtx) == &XINT (in_rtx, i) + && XINT (in_rtx, i) >= 0) + return insn_name_ptr[XINT (in_rtx, i)]; + } + + return NULL; } ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: A patch for PPro 1998-05-06 17:12 ` A patch for PPro H.J. Lu @ 1998-05-06 18:14 ` Jeffrey A Law 1998-05-07 15:31 ` Jim Wilson 1 sibling, 0 replies; 22+ messages in thread From: Jeffrey A Law @ 1998-05-06 18:14 UTC (permalink / raw) To: H.J. Lu; +Cc: Jim Wilson, scox, crux, egcs In message < m0yX9fY-000268C@ocean.lucon.org >you write: > The current egcs in CVS disables fp conditional move for PPro. The > problem is mark_constants has to search inside a CONST_DOUBLE when it > is enabled. I understand it is not safe to do so. I am enclosing a > patch which will search inside a CONST_DOUBLE only for those PPro fp > conditional moves. It won't affect any other CPUs, even other x86 > instructions. The problem with conditional moves is much worse. Jim outlined the problems yesterday I believe. You can also read the short comment before the floating point conditional moves. jeff ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: A patch for PPro 1998-05-06 17:12 ` A patch for PPro H.J. Lu 1998-05-06 18:14 ` Jeffrey A Law @ 1998-05-07 15:31 ` Jim Wilson 1 sibling, 0 replies; 22+ messages in thread From: Jim Wilson @ 1998-05-07 15:31 UTC (permalink / raw) To: H.J. Lu; +Cc: law, scox, crux, egcs I believe this patch is the wrong approach, because back-end files should not be trying to use private data which is used in varasm.c internals. That destroys modularity, and makes it harder to maintain gcc. If we really want to take this route, then we need to make this data public rather than private, instead of adding hooks to allow other code to use varasm.c's private data. This situation is a little confusing because this private data is not stored in static variables inside varasm.c, but rather in the global RTL, but it is still private data. In any case, I don't believe that this really fixes the problem. There is a general problem that we can't support input reloads with an insn using cc0, and there is no safe way to prevent generation of input reloads. I think the only safe fix is to modify the conditional move patterns to emit both the compare and the conditional move, and split them after reload. Jim ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: More fp bug in egcs
@ 1998-05-03 0:55 H.J. Lu
1998-05-03 12:03 ` H.J. Lu
1998-05-03 20:10 ` Jim Wilson
0 siblings, 2 replies; 22+ messages in thread
From: H.J. Lu @ 1998-05-03 0:55 UTC (permalink / raw)
To: wilson, law, scox, p3; +Cc: egcs
>
> >
> > I believe this is another bug in the same i386 code as my last patch.
> >
> > The problem is that the only FP->DImode converstion instruction pops the
> > FP stack. Normally we have both popping and non-popping versions of
> > instructions. The x86 code handles this by aborting if we need to emit
> > the non-existent non-popping instruction. However, this can't work all
> > of the time, because it assumes the optimizer always generates optimal
> > code. That isn't safe. And it is obviously not safe if we are compiling
> > without optimization.
> >
> > In order to fix this, we need to emulate the missing instruction if gcc
> > needs to emit it. The following patch does this. If there is a better way
> > to do this, then let me know.
> >
> > Thu Apr 30 19:28:16 1998 Jim Wilson <wilson@cygnus.com>
> >
> > * i386.c (output_fix_trunc): Add code to emulate non-popping DImode
> > case.
> >
> > *** i386.c Sun Feb 15 11:54:11 1998
> > --- /home/wilson/tmp/i386.c Thu Apr 30 19:26:54 1998
> > *************** output_fix_trunc (insn, operands)
> > *** 3731,3738 ****
> > int stack_top_dies = find_regno_note (insn, REG_DEAD, FIRST_STACK_REG) != 0;
> > rtx xops[2];
> >
> > ! if (! STACK_TOP_P (operands[1]) ||
> > ! (GET_MODE (operands[0]) == DImode && ! stack_top_dies))
> > abort ();
> >
> > xops[0] = GEN_INT (12);
> > --- 3731,3737 ----
> > int stack_top_dies = find_regno_note (insn, REG_DEAD, FIRST_STACK_REG) != 0;
> > rtx xops[2];
> >
> > ! if (! STACK_TOP_P (operands[1]))
> > abort ();
> >
> > xops[0] = GEN_INT (12);
> > *************** output_fix_trunc (insn, operands)
> > *** 3750,3755 ****
> > --- 3749,3765 ----
> > {
> > if (stack_top_dies)
> > output_asm_insn (AS1 (fistp%z0,%0), operands);
> > + else if (GET_MODE (operands[0]) == DImode && ! stack_top_dies)
> > + {
> > + /* There is no DImode version of this without a stack pop, so
> > + we must emulate it. It doesn't matter much what the second
> > + instruction is, because the value being pushed on the FP stack
> > + is not used except for the following stack popping store.
> > + This case can only happen without optimization, so it doesn't
> > + matter that it is inefficient. */
> > + output_asm_insn (AS1 (fistp%z0,%0), operands);
> > + output_asm_insn (AS1 (fild%z0,%0), operands);
> > + }
> > else
> > output_asm_insn (AS1 (fist%z0,%0), operands);
> > }
> >
>
> Here is the trimmed down test case. I am not sure if your patch is
> correct. If you take look at the stack RTL dump, you will see SF 1 in
>
> (define_insn ""
> [(set (match_operand:DI 0 "nonimmediate_operand" "=rm")
> (fix:DI (fix:SF (match_operand:SF 1 "register_operand" "+f"))))
> (clobber (match_dup 1))
> (clobber (match_operand:SI 2 "memory_operand" "m"))
> (clobber (match_operand:DI 3 "memory_operand" "m"))
> (clobber (match_scratch:SI 4 "=&q"))]
> "TARGET_80387"
> "* return output_fix_trunc (insn, operands);")
>
> is used as the input for the next insn:
>
> ;; Insn is not within a basic block
> (insn:QI 104 269 272 (parallel[
> (set (mem:DI (plus:SI (reg:SI 6 %ebp)
> (const_int -144)))
> (fix:DI (fix:SF (reg:SF 8 %st(0)))))
> (clobber (reg:SF 8 %st(0)))
> (clobber (mem:SI (plus:SI (reg:SI 6 %ebp)
> (const_int -4))))
> (clobber (mem:DI (plus:SI (reg:SI 6 %ebp)
> (const_int -12))))
> (clobber (reg:SI 1 %edx))
> ] ) 117 {fix_truncxfsi2-1} (nil)
> (nil))
>
> ;; Insn is not within a basic block
> (insn:QI 272 104 275 (set (mem:SF (plus:SI (reg:SI 6 %ebp)
> (const_int -148)))
> (reg:SF 8 %st(0))) -1 (nil)
> (expr_list:REG_DEAD (reg:DF 8 %st(0))
> (nil)))
>
> I don't know if it is correct. Did gcc know %st(0) was not the same
> %st(0) before?
>
How does this patch look? It works for my test case.
I think it is a reload bug. I don't know what the purpose to output the
last reload for a dead register. I don't what the best solution is and
I am not sure if my patch covers all cases.
Thanks.
---
Sun May 3 00:35:41 1998 H.J. Lu (hjl@gnu.org)
* reload1.c (emit_reload_insns): Don't output the last reload
insn if OLD is dead at the end of INSN.
--- ../../../import/egcs/gcc/reload1.c Mon Apr 20 08:23:47 1998
+++ reload1.c Sun May 3 00:49:52 1998
@@ -6729,7 +6729,8 @@ emit_reload_insns (insn)
#endif
/* Output the last reload insn. */
- if (! special)
+ if (! special && (GET_CODE (old) != REG
+ || !dead_or_set_p (insn, old)))
gen_reload (old, reloadreg, reload_opnum[j],
reload_when_needed[j]);
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: More fp bug in egcs 1998-05-03 0:55 More fp bug in egcs H.J. Lu @ 1998-05-03 12:03 ` H.J. Lu 1998-05-03 17:14 ` Jim Wilson 1998-05-03 20:10 ` Jim Wilson 1 sibling, 1 reply; 22+ messages in thread From: H.J. Lu @ 1998-05-03 12:03 UTC (permalink / raw) To: H.J. Lu; +Cc: wilson, law, scox, p3, > > How does this patch look? It works for my test case. > > I think it is a reload bug. I don't know what the purpose to output the > last reload for a dead register. I don't what the best solution is and > I am not sure if my patch covers all cases. > Here is a revised patch. I hope this bug can be fixed in 1.0.3. Thanks. H.J. ---- Sun May 3 10:48:40 1998 H.J. Lu (hjl@gnu.org) * reload1.c (emit_reload_insns): Don't output the last reload insn if OLD is not the dest of INSN and is dead at the end of INSN. --- ../../../import/egcs/gcc/reload1.c Mon Apr 20 08:23:47 1998 +++ ./reload1.c Sun May 3 11:26:28 1998 @@ -6730,8 +6730,17 @@ emit_reload_insns (insn) /* Output the last reload insn. */ if (! special) - gen_reload (old, reloadreg, reload_opnum[j], - reload_when_needed[j]); + { + rtx set; + + /* Don't output the last reload if OLD is not the dest + of INSN and is dead at the end of INSN. */ + if (GET_CODE (old) != REG || !(set = single_set (insn)) + || old == SET_DEST (set) + || !dead_or_set_p (insn, old)) + gen_reload (old, reloadreg, reload_opnum[j], + reload_when_needed[j]); + } #ifdef PRESERVE_DEATH_INFO_REGNO_P /* If final will look at death notes for this reg, ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: More fp bug in egcs 1998-05-03 12:03 ` H.J. Lu @ 1998-05-03 17:14 ` Jim Wilson 1998-05-04 11:17 ` H.J. Lu 0 siblings, 1 reply; 22+ messages in thread From: Jim Wilson @ 1998-05-03 17:14 UTC (permalink / raw) To: H.J. Lu; +Cc: law, scox, crux, egcs One reason for keeping output reloads, even though the register is dead, is for debugging purposes. With your patch, it looks like debugging of unoptimized code would not work right. It would be OK to do this when optimizing, but that would not fix the x86 bug. Jim ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: More fp bug in egcs 1998-05-03 17:14 ` Jim Wilson @ 1998-05-04 11:17 ` H.J. Lu 1998-05-04 18:07 ` Jeffrey A Law 0 siblings, 1 reply; 22+ messages in thread From: H.J. Lu @ 1998-05-04 11:17 UTC (permalink / raw) To: Jim Wilson; +Cc: hjl, law, scox, crux, egcs > > One reason for keeping output reloads, even though the register is dead, is > for debugging purposes. With your patch, it looks like debugging of > unoptimized code would not work right. It would be OK to do this > when optimizing, but that would not fix the x86 bug. > If the register is clobbered, isn't the value in it random? If it is tru, how can it help debugging? I changed my patch to omit the output reload if it is in the source and is clobbered. Does it look safe? -- H.J. Lu (hjl@gnu.org) ---- Sun May 3 18:44:40 1998 H.J. Lu (hjl@gnu.org) * reload1.c (emit_reload_insns): Don't output the last reload insn if OLD is the src of INSN and is clobbered by INSN. --- ../../../import/egcs/gcc/reload1.c Mon Apr 20 08:23:47 1998 +++ ./reload1.c Sun May 3 18:57:26 1998 @@ -6730,8 +6730,17 @@ emit_reload_insns (insn) /* Output the last reload insn. */ if (! special) - gen_reload (old, reloadreg, reload_opnum[j], - reload_when_needed[j]); + { + rtx set; + + /* Don't output the last reload if OLD is the src of INSN + and is clobbered by INSN. */ + if (GET_CODE (old) != REG || !(set = single_set (insn)) + || !reg_mentioned_p (old, SET_SRC (set)) + || !regno_clobbered_p (REGNO (old), insn)) + gen_reload (old, reloadreg, reload_opnum[j], + reload_when_needed[j]); + } #ifdef PRESERVE_DEATH_INFO_REGNO_P /* If final will look at death notes for this reg, ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: More fp bug in egcs 1998-05-04 11:17 ` H.J. Lu @ 1998-05-04 18:07 ` Jeffrey A Law 1998-05-04 18:07 ` H.J. Lu 0 siblings, 1 reply; 22+ messages in thread From: Jeffrey A Law @ 1998-05-04 18:07 UTC (permalink / raw) To: H.J. Lu; +Cc: Jim Wilson, scox, crux, egcs In message < m0yWMtF-000268C@ocean.lucon.org >you write: > > > > One reason for keeping output reloads, even though the register is dead, > is > > for debugging purposes. With your patch, it looks like debugging of > > unoptimized code would not work right. It would be OK to do this > > when optimizing, but that would not fix the x86 bug. > > > > If the register is clobbered, isn't the value in it random? If it is > tru, how can it help debugging? I changed my patch to omit the output > reload if it is in the source and is clobbered. Does it look safe? From the compiler's standpoint it is clobbered. However, the value in the clobbered register might have meaning to the user that is debugging code. jeff ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: More fp bug in egcs 1998-05-04 18:07 ` Jeffrey A Law @ 1998-05-04 18:07 ` H.J. Lu 1998-05-04 22:00 ` Jeffrey A Law 1998-05-08 16:04 ` Jeffrey A Law 0 siblings, 2 replies; 22+ messages in thread From: H.J. Lu @ 1998-05-04 18:07 UTC (permalink / raw) To: law; +Cc: hjl, wilson, scox, crux, egcs > > > In message < m0yWMtF-000268C@ocean.lucon.org >you write: > > > > > > One reason for keeping output reloads, even though the register is dead, > > is > > > for debugging purposes. With your patch, it looks like debugging of > > > unoptimized code would not work right. It would be OK to do this > > > when optimizing, but that would not fix the x86 bug. > > > > > > > If the register is clobbered, isn't the value in it random? If it is > > tru, how can it help debugging? I changed my patch to omit the output > > reload if it is in the source and is clobbered. Does it look safe? > >From the compiler's standpoint it is clobbered. > > However, the value in the clobbered register might have meaning to > the user that is debugging code. > Since the value in the clobbered register is controlled by the compiler, may depend on the user source code, may change from release to release and the clobbered register itself may disappear from release to release, how will the user know what the value in the clobbered register should be? It seems to me that as far as the user is concerned, any value in the clobbered register is be ok as long as the destination of the instruction is valid. I am enclosing a modified patch. It is very conservative. Does it look safe? -- Mon May 4 07:43:05 1998 H.J. Lu (hjl@gnu.org) * reload1.c (emit_reload_insns): Don't output the last reload insn if OLD is not the dest of NSN and is in the src and is clobbered by INSN. --- ../../../import/egcs/gcc/reload1.c Mon Apr 20 08:23:47 1998 +++ ./reload1.c Mon May 4 13:41:46 1998 @@ -6730,8 +6730,18 @@ emit_reload_insns (insn) /* Output the last reload insn. */ if (! special) - gen_reload (old, reloadreg, reload_opnum[j], - reload_when_needed[j]); + { + rtx set; + + /* Don't output the last reload if OLD is not the dest of + INSN and is in the src and is clobbered by INSN. */ + if (GET_CODE (old) != REG || !(set = single_set (insn)) + || rtx_equal_p (old, SET_DEST (set)) + || !reg_mentioned_p (old, SET_SRC (set)) + || !regno_clobbered_p (REGNO (old), insn)) + gen_reload (old, reloadreg, reload_opnum[j], + reload_when_needed[j]); + } #ifdef PRESERVE_DEATH_INFO_REGNO_P /* If final will look at death notes for this reg, ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: More fp bug in egcs 1998-05-04 18:07 ` H.J. Lu @ 1998-05-04 22:00 ` Jeffrey A Law 1998-05-08 16:04 ` Jeffrey A Law 1 sibling, 0 replies; 22+ messages in thread From: Jeffrey A Law @ 1998-05-04 22:00 UTC (permalink / raw) To: H.J. Lu; +Cc: wilson, scox, crux, egcs In message < m0yWSyp-000268C@ocean.lucon.org >you write: > Since the value in the clobbered register is controlled by the > compiler, may depend on the user source code, may change from > release to release and the clobbered register itself may disappear > from release to release, how will the user know what the value in > the clobbered register should be? It seems to me that as far as > the user is concerned, any value in the clobbered register is > be ok as long as the destination of the instruction is valid. HJ, we are not fixing this bug with a patch to reload. Period. If you want to argue that this code is an optimization and it actually helps with -O, then we'll consider adding it as an optimization. But it is not the right way to fix the bug. jef ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: More fp bug in egcs 1998-05-04 18:07 ` H.J. Lu 1998-05-04 22:00 ` Jeffrey A Law @ 1998-05-08 16:04 ` Jeffrey A Law 1 sibling, 0 replies; 22+ messages in thread From: Jeffrey A Law @ 1998-05-08 16:04 UTC (permalink / raw) To: H.J. Lu; +Cc: wilson, scox, crux, egcs In message < m0yWSyp-000268C@ocean.lucon.org >you write: > * reload1.c (emit_reload_insns): Don't output the last reload > insn if OLD is not the dest of NSN and is in the src and is > clobbered by INSN. I played with this a little the other day on the assumption that we could consider it as an optimization patch. I did bootstraps with an instrumented compiler to detect when this optimization could be applied. For an x86 -O2 bootstrap it triggered only about 15 times; I analyzed at some (but not all) of the cases where this change was able to optimize code better. They all looked like safe optimizations (which isn't a suprise given the purpose of the patch). Sometimes we deleted a store into memory other times it was just a reg-reg copy. The optimization never triggered during a PA bootstrap. Which, isn't a big suprise. I went ahead and installed a modified version of the patch which only triggers when -fexpensive-optimizations is enabled. jeff ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: More fp bug in egcs 1998-05-03 0:55 More fp bug in egcs H.J. Lu 1998-05-03 12:03 ` H.J. Lu @ 1998-05-03 20:10 ` Jim Wilson 1998-05-05 5:46 ` Jeffrey A Law 1 sibling, 1 reply; 22+ messages in thread From: Jim Wilson @ 1998-05-03 20:10 UTC (permalink / raw) To: H.J. Lu; +Cc: law, scox, crux, egcs How does this patch look? It works for my test case. A reload patch is not a safe way to fix this problem. Your reload patch affects all programs for all targets. My patch affects only x86 programs that currently fail. Since my patch has much more limited scope, it is a much better solution for a egcs-1.0.3 bug fix release. I think it is a reload bug. I don't know what the purpose to output the last reload for a dead register. I don't what the best solution is and I am not sure if my patch covers all cases. I think it is more accurate to call it a `missed optimization' than a `bug', as the output is correct. The only reason it fails on the x86 is because of a i386.md bug which my patch fixes. While it may be useful to add this optimization, I don't think that egcs-1.0.3 is the proper place to try it. Especially considering how hard it is to get reload patches right. Jim ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: More fp bug in egcs 1998-05-03 20:10 ` Jim Wilson @ 1998-05-05 5:46 ` Jeffrey A Law 0 siblings, 0 replies; 22+ messages in thread From: Jeffrey A Law @ 1998-05-05 5:46 UTC (permalink / raw) To: Jim Wilson; +Cc: H.J. Lu, scox, crux, egcs In message < 199805032254.PAA08319@rtl.cygnus.com >you write: > How does this patch look? It works for my test case. > > A reload patch is not a safe way to fix this problem. Your reload patch > affects all programs for all targets. My patch affects only x86 programs > that currently fail. Since my patch has much more limited scope, it is a > much better solution for a egcs-1.0.3 bug fix release. Agreed! > I think it is a reload bug. I don't know what the purpose to output the > last reload for a dead register. I don't what the best solution is and > I am not sure if my patch covers all cases. > > I think it is more accurate to call it a `missed optimization' than a `bug', > as the output is correct. The only reason it fails on the x86 is because of > a i386.md bug which my patch fixes. Agreed again. HJ -- you're making the same fundamental mistake as you did with the "disable regmove" bug which really turned out to be a bug in i386.md. Jim's patch is more correct to fix the bug and several orders of magnitude safer. And I think both Jim and I agree that you can make a case for your patch as an optimization -- in which case it would belong in the development branch, not the release branch. jeff ^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~1998-05-08 16:04 UTC | newest] Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 1998-04-25 18:25 More fp bug in egcs H.J. Lu 1998-04-27 21:29 ` Jim Wilson 1998-04-30 20:03 ` Jim Wilson 1998-05-02 18:56 ` H.J. Lu 1998-05-03 20:10 ` Jim Wilson 1998-05-05 0:35 ` Jeffrey A Law 1998-05-05 19:14 ` H.J. Lu 1998-05-06 11:49 ` Jim Wilson 1998-05-05 5:03 ` Jeffrey A Law 1998-05-06 17:12 ` A patch for PPro H.J. Lu 1998-05-06 18:14 ` Jeffrey A Law 1998-05-07 15:31 ` Jim Wilson 1998-05-03 0:55 More fp bug in egcs H.J. Lu 1998-05-03 12:03 ` H.J. Lu 1998-05-03 17:14 ` Jim Wilson 1998-05-04 11:17 ` H.J. Lu 1998-05-04 18:07 ` Jeffrey A Law 1998-05-04 18:07 ` H.J. Lu 1998-05-04 22:00 ` Jeffrey A Law 1998-05-08 16:04 ` Jeffrey A Law 1998-05-03 20:10 ` Jim Wilson 1998-05-05 5:46 ` Jeffrey A Law
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).