* A bug in mark_constants () in varasm.c @ 1998-03-13 18:47 H.J. Lu 1998-03-16 20:44 ` Jim Wilson 0 siblings, 1 reply; 19+ messages in thread From: H.J. Lu @ 1998-03-13 18:47 UTC (permalink / raw) To: egcs With egcs 980312 09:16am PST configured for linux/x86. I got the undefined label ".LC1": # cc -S -O2 -march=i686 -B/home/work/gnu/bin/egcs/gcc/ -dra r_tanh.c # gcc -c r_tanh.s # r_tanh.o U .LC1 00000000 t gcc2_compiled. 00000000 T tanh The problem is mark_constants () in varasm.c. Here is the comment in mark_constants (): /* Never search inside a CONST_DOUBLE, because CONST_DOUBLE_MEM may be 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) return; As you can see in r_tanh.c.stack, .LC1 is used. But mark_constants () doesn't search inside CONST_DOUBLE. That causes the problem. Could someone please fix it? Thanks. -- H.J. Lu (hjl@gnu.org) ---r_tanh.c--- extern __inline double __expm1 (double __x) { double __temp; __temp = 1.0; return __temp; } extern __inline double __sgn1 (double __x) { return __x >= 0.0 ? 1.0 : -1.0; } double tanh (double __x) { return __expm1 (__x) * __sgn1 (-__x); } ----r_tanh.c.stack---- ;; Function tanh (note 2 0 5 "" NOTE_INSN_DELETED) (note 5 2 6 "" NOTE_INSN_FUNCTION_BEG) (note 6 5 8 "" NOTE_INSN_DELETED) (note 8 6 9 "" NOTE_INSN_BLOCK_BEG) (note 9 8 10 "" NOTE_INSN_BLOCK_BEG) (note/i 10 9 12 "" NOTE_INSN_BLOCK_BEG) (note/i 12 10 15 "" NOTE_INSN_DELETED) (note/i 15 12 17 "" NOTE_INSN_BLOCK_END) (note 17 15 21 "" NOTE_INSN_BLOCK_END) (note 21 17 48 "" NOTE_INSN_BLOCK_BEG) (note 48 21 52 "" NOTE_INSN_DELETED) (note 52 48 54 "" NOTE_INSN_DELETED) (note 54 52 36 "" NOTE_INSN_DELETED) (note 36 54 39 "" NOTE_INSN_BLOCK_END) (note 39 36 44 "" NOTE_INSN_DELETED) (note 44 39 55 "" NOTE_INSN_BLOCK_END) (note 55 44 57 "" NOTE_INSN_DELETED) ;; Insn is not within a basic block (insn 57 55 59 (set (mem:SI (pre_dec:SI (reg:SI 7 %esp))) (reg:SI 6 %ebp)) 49 {movsi-2} (nil) (nil)) ;; Insn is not within a basic block (insn 59 57 56 (set (reg:SI 6 %ebp) (reg:SI 7 %esp)) 53 {movsi+2} (insn_list 57 (nil)) (nil)) ;; Start of basic block 0, registers live: 6 [bp] (note 56 59 4 "" NOTE_INSN_PROLOGUE_END) (insn:QI 4 56 18 (set (reg:DF 8 %st(0)) (mem:DF (plus:SI (reg:SI 6 %ebp) (const_int 8)))) 76 {movdf_mem+1} (insn_list 59 (insn_list 57 (nil))) (expr_list:REG_EQUIV (mem:DF (plus:SI (reg:SI 6 %ebp) (const_int 8))) (nil))) (insn:QI 18 4 22 (set (reg:DF 8 %st(0)) (neg:DF (reg:DF 8 %st(0)))) 197 {negdf2} (insn_list 4 (nil)) (nil)) (insn/i:QI 22 18 65 (set (reg:DF 8 %st(0)) (const_double:DF (cc0) 0 0 0)) 76 {movdf_mem+1} (nil) (expr_list:REG_EQUIV (const_double:DF (cc0) 0 0 0) (nil))) (insn:QI 65 22 49 (parallel[ (set (reg:DF 9 %st(1)) (reg:DF 8 %st(0))) (set (reg:DF 8 %st(0)) (reg:DF 9 %st(1))) ] ) -1 (nil) (nil)) (insn:QI 49 65 50 (parallel[ (set (cc0) (compare (reg:DF 8 %st(0)) (reg:DF 9 %st(1)))) (clobber (reg:HI 0 %ax)) ] ) 25 {cmpsf_cc_1-7} (insn_list 18 (insn_list 22 (nil))) (expr_list:REG_DEAD (reg:DF 9 %st(1)) (expr_list:REG_DEAD (reg:DF 8 %st(0)) (expr_list:REG_UNUSED (reg:HI 0 %ax) (nil))))) (insn/s:QI 50 49 64 (set (reg:DF 8 %st(0)) (if_then_else:DF (ge (cc0) (const_int 0)) (const_double:DF (cc0) 0 0 1073709056) (const_double:DF (mem/u:DF (symbol_ref/u:SI ("*.LC1"))) 0 0 -1073774592))) 380 {movdfcc_1} (insn_list 22 (insn_list 18 (nil))) (nil)) (note 64 50 61 "" NOTE_INSN_EPILOGUE_BEG) (insn 61 64 62 (parallel[ (set (reg:SI 7 %esp) (reg:SI 6 %ebp)) (clobber (reg:SI 6 %ebp)) ] ) 343 {epilogue_set_stack_ptr} (insn_list:REG_DEP_OUTPUT 57 (insn_list 59 (insn_list:REG_DEP_ANTI 4 (nil)))) (nil)) (insn 62 61 40 (parallel[ (set (reg:SI 6 %ebp) (mem:SI (reg:SI 7 %esp))) (set (reg:SI 7 %esp) (plus:SI (reg:SI 7 %esp) (const_int 4))) ] ) 345 {pop} (insn_list 57 (insn_list 61 (nil))) (nil)) (insn:QI 40 62 63 (use (reg/i:DF 8 %st(0))) -1 (insn_list 50 (nil)) (nil)) (jump_insn/s 63 40 60 (return) 334 {return_internal} (insn_list:REG_DEP_ANTI 62 (insn_list 50 (nil))) (nil)) ;; End of basic block 0 (barrier 60 63 0) ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: A bug in mark_constants () in varasm.c 1998-03-13 18:47 A bug in mark_constants () in varasm.c H.J. Lu @ 1998-03-16 20:44 ` Jim Wilson 1998-03-17 8:42 ` H.J. Lu 1998-03-17 11:33 ` H.J. Lu 0 siblings, 2 replies; 19+ messages in thread From: Jim Wilson @ 1998-03-16 20:44 UTC (permalink / raw) To: H.J. Lu; +Cc: egcs In order to reproduce this problem, I have to install your earlier i386.md patch to handle CONST_DOUBLE in the mov*cc patterns. The problem here isn't a bug in varasm.c, it is a bug in your i386.md patch. You have this: default: operands[3] = XEXP (operands[3], 0); output_asm_insn (AS1 (fld%z3,%y3), operands); where operands[3] is a CONST_DOUBLE. You are trying to fix an invalid CONST_DOUBLE by loading it from CONST_DOUBLE_MEM, but that is wrong. CONST_DOUBLE_MEM is an internal field for use by the compiler; it is not valid for a md file to use it like this. It is not a MEM equivalent to the constant as you seem to think it is. If a CONST_DOUBLE is invalid here, then the correct way to fix it is to change the predicates and/or constraints to indicate that a CONST_DOUBLE is not valid here. That will cause reload to replace the CONST_DOUBLE with a MEM, and that MEM will be valid (unlike CONST_DOUBLE_MEM). I'd suggest something like the following patch. This generates RTL that looks OK but it fails in reg-stack.c. Offhand, I don't know why. Index: i386.md =================================================================== RCS file: /egcs/carton/cvsfiles/egcs/gcc/config/i386/i386.md,v retrieving revision 1.13 diff -p -r1.13 i386.md *** i386.md 1998/03/12 12:43:42 1.13 --- i386.md 1998/03/17 03:40:09 *************** byte_xor_operation: *** 7524,7548 **** RET; }") (define_expand "movsfcc" [(match_dup 4) (set (match_operand 0 "general_operand" "") (if_then_else:SF (match_operand 1 "comparison_operator" "") ! (match_operand:SF 2 "general_operand" "") ! (match_operand:SF 3 "general_operand" "")))] "TARGET_CMOVE" " { - int i; - - for (i = 2; i <= 3; i++) - { - if ((reload_in_progress | reload_completed) == 0 - && CONSTANT_P (operands[i])) - { - operands[i] = force_const_mem (SFmode, operands[i]); - } - } operands[4] = i386_compare_gen (i386_compare_op0, i386_compare_op1); }") --- 7524,7545 ---- RET; }") + ;; ??? 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 "general_operand" "") (if_then_else:SF (match_operand 1 "comparison_operator" "") ! (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); }") *************** byte_xor_operation: *** 7550,7570 **** [(match_dup 4) (set (match_operand 0 "register_operand" "t") (if_then_else:DF (match_operand 1 "comparison_operator" "") ! (match_operand:DF 2 "general_operand" "") ! (match_operand:DF 3 "general_operand" "")))] "TARGET_CMOVE" " { - int i; - - for (i = 2; i <= 3; i++) - { - if ((reload_in_progress | reload_completed) == 0 - && CONSTANT_P (operands[i])) - { - operands[i] = force_const_mem (DFmode, operands[i]); - } - } operands[4] = i386_compare_gen (i386_compare_op0, i386_compare_op1); }") --- 7547,7557 ---- [(match_dup 4) (set (match_operand 0 "register_operand" "t") (if_then_else:DF (match_operand 1 "comparison_operator" "") ! (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); }") *************** byte_xor_operation: *** 7572,7592 **** [(match_dup 4) (set (match_operand 0 "register_operand" "t") (if_then_else:XF (match_operand 1 "comparison_operator" "") ! (match_operand:XF 2 "general_operand" "") ! (match_operand:XF 3 "general_operand" "")))] "TARGET_CMOVE" " { - int i; - - for (i = 2; i <= 3; i++) - { - if ((reload_in_progress | reload_completed) == 0 - && CONSTANT_P (operands[i])) - { - operands[i] = force_const_mem (XFmode, operands[i]); - } - } operands[4] = i386_compare_gen (i386_compare_op0, i386_compare_op1); }") --- 7559,7569 ---- [(match_dup 4) (set (match_operand 0 "register_operand" "t") (if_then_else:XF (match_operand 1 "comparison_operator" "") ! (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); }") *************** byte_xor_operation: *** 7594,7601 **** [(set (match_operand:SF 0 "general_operand" "=f,=f,=f,=f") (if_then_else:SF (match_operator 1 "comparison_operator" [(cc0) (const_int 0)]) ! (match_operand:SF 2 "general_operand" "0,f,f,fFm") ! (match_operand:SF 3 "general_operand" "f,0,f,fFm")))] "TARGET_CMOVE" "* { --- 7571,7578 ---- [(set (match_operand:SF 0 "general_operand" "=f,=f,=f,=f") (if_then_else:SF (match_operator 1 "comparison_operator" [(cc0) (const_int 0)]) ! (match_operand:SF 2 "nonimmediate_operand" "0,f,f,fm") ! (match_operand:SF 3 "nonimmediate_operand" "f,0,f,fm")))] "TARGET_CMOVE" "* { *************** byte_xor_operation: *** 7650,7657 **** [(set (match_operand:DF 0 "general_operand" "=f,=f,=f,=f") (if_then_else:DF (match_operator 1 "comparison_operator" [(cc0) (const_int 0)]) ! (match_operand:DF 2 "general_operand" "0,f,f,fFm") ! (match_operand:DF 3 "general_operand" "f,0,f,fFm")))] "TARGET_CMOVE" "* { --- 7627,7634 ---- [(set (match_operand:DF 0 "general_operand" "=f,=f,=f,=f") (if_then_else:DF (match_operator 1 "comparison_operator" [(cc0) (const_int 0)]) ! (match_operand:DF 2 "nonimmediate_operand" "0,f,f,fm") ! (match_operand:DF 3 "nonimmediate_operand" "f,0,f,fm")))] "TARGET_CMOVE" "* { *************** byte_xor_operation: *** 7706,7713 **** [(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 "register_operand" "0,f,f,fFm") ! (match_operand:XF 3 "register_operand" "f,0,f,fFm")))] "TARGET_CMOVE" "* { --- 7683,7690 ---- [(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,fm") ! (match_operand:XF 3 "nonimmediate_operand" "f,0,f,fm")))] "TARGET_CMOVE" "* { ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: A bug in mark_constants () in varasm.c 1998-03-16 20:44 ` Jim Wilson @ 1998-03-17 8:42 ` H.J. Lu 1998-03-18 14:08 ` Jim Wilson 1998-03-17 11:33 ` H.J. Lu 1 sibling, 1 reply; 19+ messages in thread From: H.J. Lu @ 1998-03-17 8:42 UTC (permalink / raw) To: Jim Wilson; +Cc: egcs, scox > > In order to reproduce this problem, I have to install your earlier i386.md > patch to handle CONST_DOUBLE in the mov*cc patterns. > > The problem here isn't a bug in varasm.c, it is a bug in your i386.md patch. > You have this: > default: > operands[3] = XEXP (operands[3], 0); > output_asm_insn (AS1 (fld%z3,%y3), operands); > where operands[3] is a CONST_DOUBLE. You are trying to fix an invalid > CONST_DOUBLE by loading it from CONST_DOUBLE_MEM, but that is wrong. > CONST_DOUBLE_MEM is an internal field for use by the compiler; it is > not valid for a md file to use it like this. It is not a MEM > equivalent to the constant as you seem to think it is. That is not my patch. I only optimized it for standard x87 constants. I didn't change anything else. > > If a CONST_DOUBLE is invalid here, then the correct way to fix it is to > change the predicates and/or constraints to indicate that a CONST_DOUBLE > is not valid here. That will cause reload to replace the CONST_DOUBLE with > a MEM, and that MEM will be valid (unlike CONST_DOUBLE_MEM). > > I'd suggest something like the following patch. This generates RTL that > looks OK but it fails in reg-stack.c. Offhand, I don't know why. > Thanks for looking into it. I'd like to hear comments from Stan first. Here is my complete patch to fix all the bugs in my 5 test cases. There are several bugs exposed in my 5 test cases: 1. FP constants in nested functions are broken on PPro. Even if you may compile the code, gcc will leave out the FP constant definitions such that there are undefined references to the FP constants. Use "nm" to see it. 2. reg-stack.c mishandles the conditional move. I will forard my analysis to you. BTW, I know my patch doesn't fix all the bugs. Jim, as you have observed that an invalid CONST_DOUBLE may still be used. Lucky for x86, only CONST0_RTX and CONST1_RTX are generated by gcc, which happen to be standard x87 constants. Thanks. H.J. ---- ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: A bug in mark_constants () in varasm.c 1998-03-17 8:42 ` H.J. Lu @ 1998-03-18 14:08 ` Jim Wilson 1998-03-18 14:08 ` H.J. Lu 0 siblings, 1 reply; 19+ messages in thread From: Jim Wilson @ 1998-03-18 14:08 UTC (permalink / raw) To: H.J. Lu; +Cc: egcs, scox That is not my patch. I only optimized it for standard x87 constants. I didn't change anything else. It is your patch that I am talking about. The code that you added to optimize for standard x87 constants is wrong. It will not work. As I mentioned before, you can not use the CONST_DOUBLE_MEM field in an assembler output template. You either need to find some way to directly load the CONST_DOUBLE into a reg, or else you should reject the CONST_DOUBLE in the constraints (for instance by changing F to G). Thanks for looking into it. I'd like to hear comments from Stan first. Here is my complete patch to fix all the bugs in my 5 test cases. There are several bugs exposed in my 5 test cases: We don't need 5 different testcases for a single bug. You ran into more than one bug because your i386.md patch was wrong, which in turn led you modify varasm.c, but your varasm.c patch is also wrong. Adding bad patches on top of bad patches will not get you anywhere. You need to remove all of your patches and start over again from the beginning, preferably doing something similar to my i386.md patch. 1. FP constants in nested functions are broken on PPro. Even if you may compile the code, gcc will leave out the FP constant definitions such that there are undefined references to the FP constants. Use "nm" to see it. No, this isn't broken. This is a consequence of problems with your patches. If you remove your patches, this problem will go away. 2. reg-stack.c mishandles the conditional move. I will forard my analysis to you. Yes, this is broken. This prevents my i386.md patch from working. I don't know much about reg-stack.c, but your reg-stack.c patch makes my i386.md patch work, and it works for all 5 of your testcases. Jim ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: A bug in mark_constants () in varasm.c 1998-03-18 14:08 ` Jim Wilson @ 1998-03-18 14:08 ` H.J. Lu 0 siblings, 0 replies; 19+ messages in thread From: H.J. Lu @ 1998-03-18 14:08 UTC (permalink / raw) To: Jim Wilson; +Cc: egcs, scox > Thanks for looking into it. I'd like to hear comments from Stan first. > Here is my complete patch to fix all the bugs in my 5 test cases. There > are several bugs exposed in my 5 test cases: > > We don't need 5 different testcases for a single bug. You ran into more than There are at least 2 bugs. I will provide some new tests to show them. You can try my reg-stack.c patch and your i386.md patch individually. > one bug because your i386.md patch was wrong, which in turn led you modify > varasm.c, but your varasm.c patch is also wrong. Adding bad patches on top > of bad patches will not get you anywhere. You need to remove all of your > patches and start over again from the beginning, preferably doing something > similar to my i386.md patch. > > 1. FP constants in nested functions are broken on PPro. Even if you may > compile the code, gcc will leave out the FP constant definitions such > that there are undefined references to the FP constants. Use "nm" to > see it. > > No, this isn't broken. This is a consequence of problems with your patches. > If you remove your patches, this problem will go away. It won't. Your i386.md patch fixes this bug. > > 2. reg-stack.c mishandles the conditional move. I will forard my > analysis to you. > > Yes, this is broken. This prevents my i386.md patch from working. > I don't know much about reg-stack.c, but your reg-stack.c patch makes my > i386.md patch work, and it works for all 5 of your testcases. > My reg-stack.c patch fixes this bug. -- H.J. Lu (hjl@gnu.org) ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: A bug in mark_constants () in varasm.c 1998-03-16 20:44 ` Jim Wilson 1998-03-17 8:42 ` H.J. Lu @ 1998-03-17 11:33 ` H.J. Lu 1 sibling, 0 replies; 19+ messages in thread From: H.J. Lu @ 1998-03-17 11:33 UTC (permalink / raw) To: Jim Wilson; +Cc: scox, egcs > > In order to reproduce this problem, I have to install your earlier i386.md > patch to handle CONST_DOUBLE in the mov*cc patterns. > > The problem here isn't a bug in varasm.c, it is a bug in your i386.md patch. > You have this: > default: > operands[3] = XEXP (operands[3], 0); > output_asm_insn (AS1 (fld%z3,%y3), operands); > where operands[3] is a CONST_DOUBLE. You are trying to fix an invalid > CONST_DOUBLE by loading it from CONST_DOUBLE_MEM, but that is wrong. > CONST_DOUBLE_MEM is an internal field for use by the compiler; it is > not valid for a md file to use it like this. It is not a MEM > equivalent to the constant as you seem to think it is. > > If a CONST_DOUBLE is invalid here, then the correct way to fix it is to > change the predicates and/or constraints to indicate that a CONST_DOUBLE > is not valid here. That will cause reload to replace the CONST_DOUBLE with > a MEM, and that MEM will be valid (unlike CONST_DOUBLE_MEM). > > I'd suggest something like the following patch. This generates RTL that > looks OK but it fails in reg-stack.c. Offhand, I don't know why. > > Index: i386.md Hi, Jim, your i386.md patch plus this reg-stack.c patch fixes all my test cases. Thanks. H.J. --- Sun Mar 15 19:25:40 1998 H.J. Lu (hjl@gnu.org) * reg-stack.c (compare_for_stack_reg): Remove the correct register from the stack when popping the dead registers for have_cmove == 1. Index: reg-stack.c =================================================================== RCS file: /home/work/cvs/gnu/egcs/gcc/reg-stack.c,v retrieving revision 1.1.1.8 diff -u -r1.1.1.8 reg-stack.c --- reg-stack.c 1998/03/12 17:18:54 1.1.1.8 +++ reg-stack.c 1998/03/16 15:55:17 @@ -2067,9 +2073,25 @@ if (src1_note) { - CLEAR_HARD_REG_BIT (regstack->reg_set, REGNO (XEXP (src1_note, 0))); + int regno = REGNO (XEXP (src1_note, 0)); + int top = regstack->top; + CLEAR_HARD_REG_BIT (regstack->reg_set, regno); replace_reg (&XEXP (src1_note, 0), FIRST_STACK_REG); regstack->top--; + if (regstack->reg [top] != regno) + { + int i; + if (! have_cmove) + abort (); + for (i = regstack->top; i >= 0; i--) + if (regstack->reg [i] == regno) + { + int j; + for (j = i; j < top; j++) + regstack->reg [j] = regstack->reg [j + 1]; + break; + } + } } /* If the second operand dies, handle that. But if the operands are @@ -2088,9 +2110,25 @@ if (get_hard_regnum (regstack, XEXP (src2_note, 0)) == FIRST_STACK_REG && src1_note) { - CLEAR_HARD_REG_BIT (regstack->reg_set, REGNO (XEXP (src2_note, 0))); + int regno = REGNO (XEXP (src2_note, 0)); + int top = regstack->top; + CLEAR_HARD_REG_BIT (regstack->reg_set, regno); replace_reg (&XEXP (src2_note, 0), FIRST_STACK_REG + 1); regstack->top--; + if (regstack->reg [top] != regno) + { + int i; + if (! have_cmove) + abort (); + for (i = regstack->top; i >= 0; i--) + if (regstack->reg [i] == regno) + { + int j; + for (j = i; j < top; j++) + regstack->reg [j] = regstack->reg [j + 1]; + break; + } + } } else { ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: A bug in mark_constants () in varasm.c @ 1998-03-19 12:55 Pal Engstad 1998-03-21 10:36 ` H.J. Lu 1998-03-21 10:36 ` Per Bothner 0 siblings, 2 replies; 19+ messages in thread From: Pal Engstad @ 1998-03-19 12:55 UTC (permalink / raw) To: egcs Hi, I really do appreciate all your efforts into making gcc such a lovely compiler, but I do think you need to follow some better programing style guidelines. Linus Torvalds requires that all code in the source is using a tab length of 8 spaces, and he also states that you are a bad programmer if you write code with more than 3 indentation levels. This has caused the code to be fairly easy to read and maintainance is pretty straight forward. When I see patches like this I frown. DISABLE THE COPY & PASTE FUNCTIONS IN YOUR EDITOR! Instead of making duplicate code in two places, make a function out of it. How hard is that? static void erase_reg_from_regstack(int regno, regstack_t regstack, int top) { if (regstack->reg [top] != regno) { int i; if (! have_cmove) abort (); for (i = regstack->top; i >= 0; i--) if (regstack->reg [i] == regno) { int j; for (j = i; j < top; j++) regstack->reg [j] = regstack->reg [j + 1]; break; } } } I don't know what type regstack is, so please bear with me. PKE. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: A bug in mark_constants () in varasm.c 1998-03-19 12:55 Pal Engstad @ 1998-03-21 10:36 ` H.J. Lu 1998-03-21 10:36 ` Per Bothner 1 sibling, 0 replies; 19+ messages in thread From: H.J. Lu @ 1998-03-21 10:36 UTC (permalink / raw) To: Pal Engstad; +Cc: egcs > > Hi, > > I really do appreciate all your efforts into making gcc such a lovely > compiler, but I do think you need to follow some better programing > style guidelines. > > Linus Torvalds requires that all code in the source is using a tab > length of 8 spaces, and he also states that you are a bad programmer > if you write code with more than 3 indentation levels. > > This has caused the code to be fairly easy to read and maintainance is > pretty straight forward. > > When I see patches like this I frown. > > DISABLE THE COPY & PASTE FUNCTIONS IN YOUR EDITOR! > > Instead of making duplicate code in two places, make a function out of > it. How hard is that? > > static void > erase_reg_from_regstack(int regno, regstack_t regstack, int top) > { > if (regstack->reg [top] != regno) > { > int i; > if (! have_cmove) > abort (); > for (i = regstack->top; i >= 0; i--) > if (regstack->reg [i] == regno) > { > int j; > for (j = i; j < top; j++) > regstack->reg [j] = regstack->reg [j + 1]; > break; > } > } > } > > I don't know what type regstack is, so please bear with me. > Done by Stan. -- H.J. Lu (hjl@gnu.org) ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: A bug in mark_constants () in varasm.c 1998-03-19 12:55 Pal Engstad 1998-03-21 10:36 ` H.J. Lu @ 1998-03-21 10:36 ` Per Bothner 1998-03-23 15:35 ` Joe Buck 1998-03-23 19:24 ` Pal Engstad 1 sibling, 2 replies; 19+ messages in thread From: Per Bothner @ 1998-03-21 10:36 UTC (permalink / raw) To: Pal Engstad; +Cc: egcs > Linus Torvalds requires that all code in the source is using a tab > length of 8 spaces, and he also states that you are a bad programmer > if you write code with more than 3 indentation levels. I didn't know that Linus Torvalds has done research on programming styles. He much have, since why else would you have the audacity quote him as an authority, on a list where there are dozens of people with more than double Linus's programming experience? > This has caused the code to be fairly easy to read and maintainance is > pretty straight forward. Irrelevant - a kernel is a lot simpler than an optimizing compiler, in terms of complexity of algorithms and data structures. --Per Bothner Cygnus Solutions bothner@cygnus.com http://www.cygnus.com/~bothner ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: A bug in mark_constants () in varasm.c 1998-03-21 10:36 ` Per Bothner @ 1998-03-23 15:35 ` Joe Buck 1998-03-23 22:59 ` Jeffrey A Law 1998-03-24 12:23 ` Wolfram Gloger 1998-03-23 19:24 ` Pal Engstad 1 sibling, 2 replies; 19+ messages in thread From: Joe Buck @ 1998-03-23 15:35 UTC (permalink / raw) To: Per Bothner; +Cc: engstad, egcs Pal Engstad writes: > > Linus Torvalds requires that all code in the source is using a tab > > length of 8 spaces, and he also states that you are a bad programmer > > if you write code with more than 3 indentation levels. Linus has also said that the GNU coding standards should be burned. Since gcc uses the GNU coding standards, it isn't going to make Linus happy. The gcc developers aren't going to reject the GNU coding standards because Linus says so. Per Bothner writes: > I didn't know that Linus Torvalds has done research on programming styles. In fairness to Linus, much of gcc is difficult to follow and to maintain in part because many of the functions are too large (especially with the #ifdefs mixed in). Furthermore, since many of the algorithms turned on at higher optimization levels cost O(n^2) or even O(n^3) in the size of the function, gcc would bootstrap faster if these were cut down a bit. And egcs was supposed to be about the gcc developers learning from the success of Linux, after all. Just the same, it's unrealistic to implement many of the algorithms in gcc with only three indentation levels, so it's just wrong to declare that as a strict rule, and arrogant to say that those who violate it are bad programmers. Sometimes it appears that Linus's success and fame has gone to his head a bit. > > This has caused the code to be fairly easy to read and maintainance is > > pretty straight forward. > > Irrelevant - a kernel is a lot simpler than an optimizing compiler, > in terms of complexity of algorithms and data structures. Well, let's say that there is some relevance (smaller functions make for ease of understanding and maintainance), but nevertheless Per is right: gcc is vastly more complicated than the Linux kernel. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: A bug in mark_constants () in varasm.c 1998-03-23 15:35 ` Joe Buck @ 1998-03-23 22:59 ` Jeffrey A Law 1998-03-24 9:27 ` Craig Burley 1998-03-31 0:46 ` Marc Espie 1998-03-24 12:23 ` Wolfram Gloger 1 sibling, 2 replies; 19+ messages in thread From: Jeffrey A Law @ 1998-03-23 22:59 UTC (permalink / raw) To: Joe Buck; +Cc: Per Bothner, engstad, egcs In message < 199803231747.JAA23318@atrus.synopsys.com >you write: > Pal Engstad writes: > > > Linus Torvalds requires that all code in the source is using a tab > > > length of 8 spaces, and he also states that you are a bad programmer > > > if you write code with more than 3 indentation levels. > > Linus has also said that the GNU coding standards should be burned. > Since gcc uses the GNU coding standards, it isn't going to make Linus > happy. The gcc developers aren't going to reject the GNU coding > standards because Linus says so. No particular one is better than the other -- and trying to convince folks that one is better than the other is an exercise in futility. It is entirely a matter of personal taste. egcs uses the GNU coding standards and that's highly unlikely to change. What is important is that there is *some* coding standard for a project of this size. Most of the actual details of the standard don't matter, just the fact that a standard exists, is documented and is followed. > In fairness to Linus, much of gcc is difficult to follow and to maintain > in part because many of the functions are too large (especially with the > #ifdefs mixed in). Furthermore, since many of the algorithms turned on at > higher optimization levels cost O(n^2) or even O(n^3) in the size of the > function, gcc would bootstrap faster if these were cut down a bit. And > egcs was supposed to be about the gcc developers learning from the success > of Linux, after all. Can't argue with any of this :-) > > Irrelevant - a kernel is a lot simpler than an optimizing compiler, > > in terms of complexity of algorithms and data structures. > > Well, let's say that there is some relevance (smaller functions make > for ease of understanding and maintainance), but nevertheless Per is > right: gcc is vastly more complicated than the Linux kernel. Depends on what you're doing. Having spent time in both the OS development world and the compiler development world, some aspects of compiler development are *far* easier -- mostly in areas related to debugging and not having to deal with async events (like interrrupts) jeff ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: A bug in mark_constants () in varasm.c 1998-03-23 22:59 ` Jeffrey A Law @ 1998-03-24 9:27 ` Craig Burley 1998-03-31 0:46 ` Marc Espie 1 sibling, 0 replies; 19+ messages in thread From: Craig Burley @ 1998-03-24 9:27 UTC (permalink / raw) To: law; +Cc: jbuck, bothner, engstad, egcs > > Well, let's say that there is some relevance (smaller functions make > > for ease of understanding and maintainance), but nevertheless Per is > > right: gcc is vastly more complicated than the Linux kernel. >Depends on what you're doing. Having spent time in both the OS >development world and the compiler development world, some aspects of >compiler development are *far* easier -- mostly in areas related to >debugging and not having to deal with async events (like interrrupts) Both of these can be, and IMO are, correct. gcc is more complicated than any OS kernel I've worked on; OS kernel development is much harder (IMO) than compiler development. The way I'd put it is, the complicated parts of OS design are "outside the code", while, for compiler design, the complicated parts have migrated, especially over the past 15 years or so, to "within the code". E.g. the gcc architecture includes various ways to describe machine architectures *and* automatically generate code to do the relevant compiler cleverness. That all amounts to directly expressing the complexity of the relevant stuff in the code itself, which makes the code itself more complicated, but developing it (hopefully) easier. Whereas, last I checked, Linux, and most OSes, don't encode the knowledge of things like interrupts, maximum service times, priorities, and so on as abstractions and include automated ways to generate the kernel from this data base of knowledge. That amounts to keeping the code "simpler", but the knowledge base that the programmer working on it must possesss necessarily is much larger, and developing the code, especially changing it to cope with new ports, is much more difficult. One of the reasons I got interested in compiler design, back when I was an OS/kernel person, was that I had a theory that kernel coding architecture could be improved so the code was not only much easier to maintain, but would produce a system both more robust and of higher overall speed if it could be based more on cleanly coded rules and descriptions than on outright coding based on implicit information. I figured the compiler field would, if that theory held any water, already be making that transition. gcc is a pretty good proof of that, so I would guess that, within 20 years, there'll be one or two industrial-strength kernels out there that include at least as much automated code for the "tough parts" as gcc now includes. tq vm, (burley) ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: A bug in mark_constants () in varasm.c 1998-03-23 22:59 ` Jeffrey A Law 1998-03-24 9:27 ` Craig Burley @ 1998-03-31 0:46 ` Marc Espie 1998-04-02 9:35 ` Jeffrey A Law 1 sibling, 1 reply; 19+ messages in thread From: Marc Espie @ 1998-03-31 0:46 UTC (permalink / raw) To: egcs Well, apart from coding standards, there are some parts that would benefit from cleaner coding. It sometimes seem that the guys who develop gcc don't believe the compiler knows how to optimize stuff :-) Just have a look at cccp.c, as I did to understand how gcc was looking up include paths, to see how difficult it was to add an helpful error message when #include <myheader> fails because I should have said #include "myheader". The main function is roughly 1000 lines long. I promptly gave up. main begins with stuff such as raising limits on stack size, or finding out a program name, including a VMS patch... ... then it reads options, proceeds with -D/-U/-A stuff... that's the point I sort of got lost. Looks like a lot of little functions waiting to get out. What is unsurprising at this point is that the part that raises limits on stack size is duplicated in genattrtab.c and toplev.c. I don't understand that. There must have been someone who wrote this code in at some point. Why is there three instances of the code ? Apart from being difficult to read, this means that almost certainly, anybody who finds a bug in this code will forget to fix it in all three places. For that matter, investigating the progname stuff further. - cccp has progname, but does not use it quite consistently. Instances of argv[0] and other stuff. - genattrtabs doesn't bother with progname, uses argv[0] heartily. As it is an internal program, probably doesn't matter. - toplev uses another way to generate progname. At first sight, looks largely equivalent, though it doesn't include the VMS patch. More instances of code that do largely the same thing, i.e., all error messages. I would like to comment that commonality of code and function would be much easier to see with less obfuscation. I can very well understand that a compiler is difficult to write, but this can of stunt makes it only harder to understand... ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: A bug in mark_constants () in varasm.c 1998-03-31 0:46 ` Marc Espie @ 1998-04-02 9:35 ` Jeffrey A Law 0 siblings, 0 replies; 19+ messages in thread From: Jeffrey A Law @ 1998-04-02 9:35 UTC (permalink / raw) To: Marc.Espie; +Cc: egcs In message < 199803310212.EAA02501@quatramaran.ens.fr >you write: > The main function is roughly 1000 lines long. I promptly gave up. > > main begins with stuff such as raising limits on stack size, or > finding out a program name, including a VMS patch... I don't consider cccp.c one of the better written modules in the compiler. In fact it's one of the worst. > What is unsurprising at this point is that the part that raises > limits on stack size is duplicated in genattrtab.c and toplev.c. Not suprising. > I don't understand that. There must have been someone who wrote this > code in at some point. Why is there three instances of the code ? They didn't take the time to break it out into a file that could be linked into the different programs where such code would be useful (the three modules you mention are all part of separate programs). > I would like to comment that commonality of code and function would be > much easier to see with less obfuscation. Can't argue with that. All I can say is we're addressing such problems as quickly as we can -- see the system.h cutover. I'm not sure what the next big "make everyone's life easier" change will be, but system.h certainly won't be the last! jeff ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: A bug in mark_constants () in varasm.c 1998-03-23 15:35 ` Joe Buck 1998-03-23 22:59 ` Jeffrey A Law @ 1998-03-24 12:23 ` Wolfram Gloger 1 sibling, 0 replies; 19+ messages in thread From: Wolfram Gloger @ 1998-03-24 12:23 UTC (permalink / raw) To: egcs > Just the same, it's unrealistic to implement many of the algorithms in > gcc with only three indentation levels, so it's just wrong to declare > that as a strict rule, and arrogant to say that those who violate it > are bad programmers. Sometimes it appears that Linus's success and > fame has gone to his head a bit. Just to set this straight: Linus never said that gcc developers were bad programmers (or please show me a quote). His posting in question was clearly marked as being `religious', and specifically about _his own_ coding standard (which is obviously aimed at kernel development): Date: Fri, 3 Feb 1995 12:38:51 +0200 From: Linus Torvalds <Linus.Torvalds@cs.Helsinki.FI> Message-Id: <199502031038.MAA27879@keos.Helsinki.FI> Subject: Re: Coding Standards, anyone? ... > I do have a standard of my own, BUT.. I know this is religious, and > when it comes to device drivers that I don't expect to be able to update > anyway and code like that, I allow almost any coding standard the author > wants to use, as he's the one that will have to keep it up. If somebody > else takes over (and the original author obviously doesn't keep it up > any more), the new person is free to change the style. I'm happy to say > that this has happened only a very few times. > > Anyway, here's my standard in a nutshell, with comments (and if I don't > always adhere to it 100%, it's only because sometimes I'm lazy too, but > you'll find it true for almost all code I write): ... > Coding style: > - not more than 4 levels of indentation, preferably not even more than > three levels. ... > There are *very* few reasons to break any of the above. Just about the > only good reason is to go beyond the function length limit: some > functions are inherently "flat" (ie only a few levels of indentation) > and obvious, but tend to be rather long just because it has a lot of > equivalent cases that need testing. This may be due to bad programming > (trying to make one function do many things), but it can also be simply > due to external circumstances (you get input you have no control over > and have to make decisions upon that). ... > Anyway, this got longer than I intended it to be, and most of it is > religious. Ignore it if you wish, but you'll be sorry ;-) I cannot see the declaration of strict rules or even arrogance in this `sermon'. One shouldn't take _everything_ Linus says too seriously. Regards, Wolfram. -- `Surf the sea, not double-u three...' wmglo@dent.med.uni-muenchen.de ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: A bug in mark_constants () in varasm.c 1998-03-21 10:36 ` Per Bothner 1998-03-23 15:35 ` Joe Buck @ 1998-03-23 19:24 ` Pal Engstad 1 sibling, 0 replies; 19+ messages in thread From: Pal Engstad @ 1998-03-23 19:24 UTC (permalink / raw) To: Per Bothner, bothner; +Cc: egcs Per Bothner wrote: > > > Linus Torvalds requires that all code in the source is using a tab > > length of 8 spaces, and he also states that you are a bad programmer > > if you write code with more than 3 indentation levels. > > I didn't know that Linus Torvalds has done research on programming styles. > He much have, since why else would you have the audacity quote him as > an authority, on a list where there are dozens of people with more > than double Linus's programming experience? > I am not using Linus as an authoroty, rather as an example. I had hoped that was rather clear. All I know is that it is important for the maintainers of a piece of code to enforce strict style guides. > > This has caused the code to be fairly easy to read and maintainance is > > pretty straight forward. > > Irrelevant - a kernel is a lot simpler than an optimizing compiler, > in terms of complexity of algorithms and data structures. > I disagree with the irrelevant part. No matter how complex algorithms and data structures are, there is no excuse of voiding good coding practices. (Well, sometimes there are, but these things should be rare.) My point still remains the same. Why duplicate code; it is not nescessary and is a pain for future development. (Fix part A, always remember to fix part B, C, D if you do that.) PKE. ^ permalink raw reply [flat|nested] 19+ messages in thread
* RE: A bug in mark_constants () in varasm.c
@ 1998-03-24 14:49 Kaz Kylheku
1998-03-27 15:18 ` Andreas Schwab
0 siblings, 1 reply; 19+ messages in thread
From: Kaz Kylheku @ 1998-03-24 14:49 UTC (permalink / raw)
To: 'Joe Buck', Per Bothner; +Cc: engstad, egcs
On Monday, March 23, 1998 9:47 AM, Joe Buck [SMTP:jbuck@synopsys.com] wrote:
> Pal Engstad writes:
> > > Linus Torvalds requires that all code in the source is using a tab
> > > length of 8 spaces, and he also states that you are a bad programmer
> > > if you write code with more than 3 indentation levels.
>
> Linus has also said that the GNU coding standards should be burned.
> Since gcc uses the GNU coding standards, it isn't going to make Linus
> happy. The gcc developers aren't going to reject the GNU coding
> standards because Linus says so.
I agree that the GNU coding standards should be burned. It's plain
silly to indent code like this:
for (;;)
{
/* whatever */
if (condition)
{
/* whatever */
Can you say ``adolescent rebellion against K&R?''
I don't believe that Linus would have said, at least not seriously, that
one who uses more than three levels of nesting is bad programmer.
There is something said about this in the brief document which describes
the Linux coding conventions. If I recall, it's to the effect that
deep nesting is rarely necessary. The whole document is rather
jestful in nature, anyway.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: A bug in mark_constants () in varasm.c 1998-03-24 14:49 Kaz Kylheku @ 1998-03-27 15:18 ` Andreas Schwab 0 siblings, 0 replies; 19+ messages in thread From: Andreas Schwab @ 1998-03-27 15:18 UTC (permalink / raw) To: Kaz Kylheku; +Cc: 'Joe Buck', Per Bothner, engstad, egcs Kaz Kylheku <kaz@cafe.net> writes: |> I agree that the GNU coding standards should be burned. It's plain |> silly to indent code like this: |> for (;;) |> { |> /* whatever */ |> if (condition) |> { |> /* whatever */ What's silly about that? Btw, the last line is indented too much. -- Andreas Schwab "And now for something schwab@issan.informatik.uni-dortmund.de completely different" schwab@gnu.org ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: A bug in mark_constants () in varasm.c
@ 1998-04-02 8:21 Christian Iseli
0 siblings, 0 replies; 19+ messages in thread
From: Christian Iseli @ 1998-04-02 8:21 UTC (permalink / raw)
To: egcs, Marc.Espie
> Well, apart from coding standards, there are some parts that would
> benefit from cleaner coding. It sometimes seem that the guys who
> develop gcc don't believe the compiler knows how to optimize stuff :-)
>
> Just have a look at cccp.c, as I did to understand how gcc was looking up
> include paths, to see how difficult it was to add an helpful error message
> when #include <myheader> fails because I should have said
> #include "myheader".
> ... (snipped)
Hmm, well... now that you have spent time analyzing the thing and rubbing our noses
in the dunk, are you going to submit patches to fix those problems ?
Christian
P.S. April fools' day... so the ;-)'s are implied...
^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~1998-04-02 9:35 UTC | newest] Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 1998-03-13 18:47 A bug in mark_constants () in varasm.c H.J. Lu 1998-03-16 20:44 ` Jim Wilson 1998-03-17 8:42 ` H.J. Lu 1998-03-18 14:08 ` Jim Wilson 1998-03-18 14:08 ` H.J. Lu 1998-03-17 11:33 ` H.J. Lu 1998-03-19 12:55 Pal Engstad 1998-03-21 10:36 ` H.J. Lu 1998-03-21 10:36 ` Per Bothner 1998-03-23 15:35 ` Joe Buck 1998-03-23 22:59 ` Jeffrey A Law 1998-03-24 9:27 ` Craig Burley 1998-03-31 0:46 ` Marc Espie 1998-04-02 9:35 ` Jeffrey A Law 1998-03-24 12:23 ` Wolfram Gloger 1998-03-23 19:24 ` Pal Engstad 1998-03-24 14:49 Kaz Kylheku 1998-03-27 15:18 ` Andreas Schwab 1998-04-02 8:21 Christian Iseli
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).