Re: [PATCH] analysis of global statics and removal associated vdefsand vuses

[Kenneth Zadeck <zadeck at naturalbridge dot com>]

Mark Mitchell wrote:

> Diego Novillo wrote:
>
> > On Fri, 2004-09-03 at 11:00, Kenneth Zadeck wrote:
> >
> >  
> >
> > > The enclosed patch essentually does everything suggested in Diego's 
> > > original comments. (I only left the single comment in the mail that 
> > > I discuss here)
> > >
> > >   
> >
> > Looks good to me now.  The timings are indeed better and in the 
> > noise. We should get pretty good results once we propagate this 
> > information
> > more into alias analysis.
> >
> > I can't really approve the patch, though.  I'd like a maintainer to go
> > over the Makefile.in and cgraph changes.
> Every single function needs to say what every single argument is for 
> and what the return value is.  The argument names should be in 
> ALL_CAPS.  Also, single-statement dependent statements, like:
>
>  if (x)
>    {
>       f();
>    }
>
> should be reformatted as:
>
>  if (x)
>     f();
>
> and the braces never go on the same line as anything else, so:
>
>   } while (v != x)
>
> in searchc is wrong.  Kenny, you'll have to go through and fix all 
> that up.
>
> Also, this:
>
> +     if (strcmp (TREE_STRING_POINTER (TREE_VALUE (link)), "memory") 
> == 0)
> is wrong; TREE_IDENTIFIERs are unique.  So, what you want is a global 
> tree:
>
>  memory_identifier = get_identifier ("memory")
>
> and then, here, you just want to do: "TREE_VALUE (link) == 
> memory_identifier".
>
> As for splay trees, I expect part of the problem is that we indirect 
> through a function pointer to do the comparisons on the nodes.  C is 
> not helpful here; no templates.  We could do macro stuff to make it 
> faster, but using a hash table might be better.
>
> I'm going to approve the patch, if you make those changes, since 
> Diego's looked it over and is happy, and since I don't see anything 
> algorithm that looks too scary.  You also need to run the regression 
> testsuite after you fix up the issues above, and you should always 
> post the platform on which you tested with your submission.
Mark,

Thanks for your review.

I will try to get a new patch out today with all of the above changes in 
it.  I am leaving for a week vacation tomorrow. 

This last issue in your review is somewhat problematic.  I will of 
course do whatever is correct.  However I copied this code from 
tree-ssa-operands.c at diego's request.  So if this is a bug, it is a 
bug that is already latent in the compiler. 

A grep for \"memory\" yields a second hit in varasm.c that to my (mostly 
untrained) eye looks like a similar mistake. 

I did not submit this to gcc bugs since things are not supposed to be 
posted to two news groups but it you are correct, there is at least one 
other bug (and possibly two) already in the compiler. 

Kenny