This is the mail archive of the
gcc@gcc.gnu.org
mailing list for the GCC project.
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.