This is the mail archive of the mailing list for the GCC project.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: gcc compile-time performance

On Sat, May 18, 2002 at 02:37:50PM +1200, Michael Hayes wrote:
Content-Description: message body text
> Zack Weinberg writes:
>  > I'd be willing to work with you to get these changes into GCC.  Can
>  > you point me at the most recent patchset?
> I've attached the latest patches for libiberty that I sent to Akim
> Demaille for use in Bison.
> There are other patches (gathering dust that I sent to gcc-patches
> several months ago) that make sbitmap.h and bitmap.h wrappers to these
> routines.

Okay, so looking over these patches I have some concerns.  Can you
address these, then we'll do another round?

1) Most importantly, these expose quite a bit of the implementation to
library users.  I don't think there should be include/[sle]bitmap.h at
all; instead, put them all in libiberty/bitmap-impl.h.  It's not clear
why there is the bitset.h/bbitset.h split.  Some of the stuff in
bbitset.h (notably enum bitset_type, and the compile time
configuration parameters) belongs in bitmap-impl.h also.

2) There are too many compile-time tuning knobs.  In practice, this is
going to be compiled once as part of libiberty and forgotten about.
Take out as many of these as you can, and turn the rest into runtime
toggles.  If you can't make something either go away or become a
runtime toggle, there's a structural problem that needs to get looked
at.  I'm talking about things like 

+ /* Initialise bitset statistics logging.  */
+ void
+ bitset_stats_init ()
+ {
+   bitset_stats = &bitset_stats_data;
+   bitset_stats_read ();
+ #endif /* BITSET_STATS  */
+ }

You can/should put all the statistics routines in their own file, but
don't make it compile-time conditional whether they do anything.  Oh,
and the statistics file name should be controllable by the caller.


+ /* Number of elements to initially allocate.  */
+ #endif

No one is ever going to bother adjusting this based on the host.
Having the #ifndef there makes people worry that they may need to look
for adjustments.  Take it out until it's actually wanted.  Ditto all
the other similar parameters.

3) I believe your ebitset is the same thing as Dan Berlin's.  Didn't
he demonstrate that that was almost always the right choice?  If so,
perhaps it should be the default.

4) The BITSET_EXECUTE() macros are asking for trouble.  Granted, the
existing EXECUTE_IF_SET_IN_* macros are also asking for trouble, but
that is not a reason to perpetrate the problem.  (In case you're
wondering what the issue is, it's that C preprocessor macros really
cannot handle code blocks as arguments.)

Lemme sketch a better way:

typedef struct {
  bitset_bindex list[BITSET_LIST_SIZE];
  bitset_bindex next;
  int num;
  int i;
} bitset_iterator;

  for( = min,							      \
      ITER.num = bitset_list(BSET, ITER.list, BITSET_LIST_SIZE, &;  \
      ITER.num;								      \
      ITER.num = bitset_list(BSET, ITER.list, BITSET_LIST_SIZE, &  \
    for(ITER.i = 0; ITER.i < ITER.num; ITER.i++)


You use this like so:

int foo(bitset b)
  bitset_iterator i;
  bitset_bindex n;

      n = BITSET_ITER_GET (i);
      /* do something here */

This avoids both problems with commas inside the code block, and
problems with name clashes between the iterator variables and other
variable declarations in scope (for example, if someone needed to nest
some bitset iterations).  You'll need to roll the 
if (_num < BITSET_LIST_SIZE) test into bitset_list /
bitset_reverse_list but that should not be a problem (store a sentinel
value in when you reach the end of the vector).


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]