RE: A bug in mark_constants () in varasm.c

RE: A bug in mark_constants () in varasm.c

[Kaz Kylheku]

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.

Re: A bug in mark_constants () in varasm.c

[Andreas Schwab]

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

Re: A bug in mark_constants () in varasm.c

[Jeffrey A Law]

  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

Re: A bug in mark_constants () in varasm.c

[Christian Iseli]

> 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...

Re: A bug in mark_constants () in varasm.c

[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". 

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...

Re: A bug in mark_constants () in varasm.c

[Wolfram Gloger]

> 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

Re: A bug in mark_constants () in varasm.c

[Craig Burley]

>  > 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)

Re: A bug in mark_constants () in varasm.c

[Jeffrey A Law]

  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

Re: A bug in mark_constants () in varasm.c

[Pal Engstad]

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.

Re: A bug in mark_constants () in varasm.c

[Joe Buck]

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.

Re: A bug in mark_constants () in varasm.c

[Per Bothner]

> 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

Re: A bug in mark_constants () in varasm.c

[H.J. Lu]

> 
> 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)

Re: A bug in mark_constants () in varasm.c

[Pal Engstad]

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.

Re: A bug in mark_constants () in varasm.c

[Jim Wilson]

	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

Re: A bug in mark_constants () in varasm.c

[H.J. Lu]

> 	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)

Re: A bug in mark_constants () in varasm.c

[H.J. Lu]

> 
> 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
 	{

Re: A bug in mark_constants () in varasm.c

[H.J. Lu]

> 
> 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.
----

Re: A bug in mark_constants () in varasm.c

[Jim Wilson]

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"
    "*
  {

A bug in mark_constants () in varasm.c

[H.J. Lu]

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)