This is the mail archive of the gcc@gcc.gnu.org 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


Zack Weinberg writes:
 > 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. 

Yeah, these should be moved back into libiberty where I originally had
them.

 > It's not clear
 > why there is the bitset.h/bbitset.h split.  

bbitset.h contains the private base bitset stuff that needs to be
exposed to the compiler but what I would like to be opaque to the
user.

bitset.h represents the actual interface.

 > Some of the stuff in
 > bbitset.h (notably enum bitset_type, and the compile time
 > configuration parameters) belongs in bitmap-impl.h also.

Essentially bbitset.h is bitmap-impl.h

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

I agree.  However, these are not part of the interface and I am unsure
what the optimal combination is.

 > 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 ()
 > + {
 > + #if BITSET_STATS
 > +   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.

The statistics stuff was meant for developers of the bitset routines
not users of the routines.  I guess we could add an interface to them.
However, we have to be sneaky to avoid a run-time penalty when they
are not required.

 > + /* Number of elements to initially allocate.  */
 > + 
 > + #ifndef EBITSET_INITIAL_SIZE
 > + #define EBITSET_INITIAL_SIZE 2
 > + #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.

OK.  I agree.

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

Yes, I implemented Dan's idea.  Dan advocated this for the dataflow
def/use info where it may be advantageous.  However, for register sets
I noticed no improvement.  Most of the time bitsets in gcc contain
zero or only one set bits.  In this case it is faster scanning a short
linked list.

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

<better way snipped>

Yes, I agree with this.  I only added the BITSET_EXECUTE macro
to minimise the changes I needed gcc.

I'll see what I can come up with.

Michael.


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