[PING] Patch to implement "Switch statement fdo reordering"
Mark Mitchell
mark@codesourcery.com
Wed Aug 6 19:54:00 GMT 2008
Edmar Wienskoski wrote:
> I completed all the changes discussed bellow. Attached is a new
> version of the patch.
Sorry I dropped the ball here.
The structure is now much better. I think that all that remains is some
tidying up.
Here are some specific comments:
> + int fdo_cost; /* Normalized profiling frequency */
This should say what is normalized relative to. If I see the value "3"
here, what does it mean? What about -7? (And if the latter doesn't
make sense, then this should be an unsigned int.)
> +static inline rtx
> +promote_short_index (tree index_expr, int unsignedp)
All functions need documentation for parameters and return value. See
other examples in GCC for appropriate conventions.
> + && fdo_switch_use_histogram_p(hist))
Formatting: need space before parenthesis. Please check the rest of
your patch for other possible occurrences of this problem.
> + /* Our histogram is limited by a fixed range,
> + exam the case list for cases out of this range */
Grammar: period after "range". Diction: "Examine", not "Exam".
> + /* Get start of range */
Period and two spaces before close of comment. Please review GNU coding
standards and look through your whole patch to fix things that do not match.
> + if (TREE_INT_CST_HIGH(cn->low) < 0)
> + {
> + count_negs += count_ranges;
> + }
No braces. See coding standards.
> + hcounters = (int *) xcalloc (1, ((int)hist->hdata.intvl.steps + 2)
> + * sizeof(int));
Should use XNEWVEC. Freeing should be done with corresponding delete macro.
> + Note that the table lookup scheme consists of specific code (5
> + instructions in the PowerPC case) plus 1 or 2 compare/branches
> + (depending on how the lower bound of the table would be handled)
Not clear. Remove reference to PowerPC (and it's Power Architecture
:-p) and explain more clearly what's going on.
Thanks,
--
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713
More information about the Gcc-patches
mailing list