Re: An x86 patch for egcs

[Mark Mitchell <mmitchell@usa.net>]

    Hi,

    This is a partial patch for the PPro bug I reported. It fixed
    loading standard x87 constants in conditional move. But it still
    leaves an undefined label in one of my test cases. My other test
    case still doesn't compile.

    Thanks.


This is not a commentary on the contents of this patch, merely its
form.  I object strongly to the kind of code-duplication in your
patch.  It appears to me that you replaced each of several similar
pieces of code with some large block of code which is also similar.  I
hope that you did this only because you weren't quite finished with
the patch, and wanted someone to help resolve the remaining problems.

This kind of code duplication is a pet peeve of mine; it makes it much
harder to maintain software, especially software as complex as gcc.
In general, if you had to change something in several places, then
there should have been some kind of abstraction (e.g. a macro or a
function) that would have allowed you to change something in only one
place.  That might be the fault of the previous author, but it's up to
us, the current authors, to correct these defects.

I hereby encourage the maintainers to reject patches that contain
unnecessary duplication, including mine.

    -- 
    H.J. Lu (hjl@gnu.org)
    --
    Thu Mar 12 08:25:19 1998  H.J. Lu  (hjl@gnu.org)

	    * config/i386/i386.md (movsfcc_1, movdfcc_1, movxfcc_1): Handle
	    loading standard x87 constants.

    Index: config/i386/i386.md
    ===================================================================
    RCS file: /home/work/cvs/gnu/egcs/gcc/config/i386/i386.md,v
    retrieving revision 1.1.1.8
    diff -u -r1.1.1.8 i386.md
    --- i386.md	1998/03/12 17:19:11	1.1.1.8
    +++ i386.md	1998/03/13 00:12:40
    @@ -7528,8 +7529,21 @@
	    output_asm_insn (AS1 (fld%z2,%y2), operands);
	   else
	     {
    -	  operands[2] = XEXP (operands[2], 0);
    -	  output_asm_insn (AS1 (fld%z2,%y2), operands);
    +	  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] = XEXP (operands[2], 0);
    +	      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]));
    @@ -7537,8 +7551,21 @@
	      output_asm_insn (AS1 (fld%z3,%y3), operands);
	   else
	    {
    -	  operands[3] = XEXP (operands[3], 0);
    -	  output_asm_insn (AS1 (fld%z3,%y3), operands);
    +	  int conval = standard_80387_constant_p (operands[3]);
    +
    +	  switch (conval)
    +	    {
    +	    case 1:
    +	      fprintf (asm_out_file, \"\\tfldz\\n\");
    +	      break;
    +	    case 2:
    +	      fprintf (asm_out_file, \"\\tfld1\\n\");
    +	      break;
    +	    default:
    +	      operands[3] = XEXP (operands[3], 0);
    +	      output_asm_insn (AS1 (fld%z3,%y3), operands);
    +	      break;
    +	   }

<more deleted>

-- 
Mark Mitchell <mmitchell@usa.net>
http://home.earthlink.net/~mbmitchell
Consulting Services Available