Fixed the stylist suggestions. Other responses below.
On Tue, Apr 24, 2012 at 10:22 PM, davi...@google.com wrote:
http://codereview.appspot.com/6099055/diff/1/loop-unroll.c
File loop-unroll.c (right):
http://codereview.appspot.com/6099055/diff/1/loop-unroll.c#newcode156
loop-unroll.c:156: static bool
An empty line here.
http://codereview.appspot.com/6099055/diff/1/loop-unroll.c#newcode182
loop-unroll.c:182: static bool
add empty line after comment.
http://codereview.appspot.com/6099055/diff/1/loop-unroll.c#newcode213
loop-unroll.c:213: /* Compute the number of branches in LOOP, weighted
by execution counts. */
same here.
http://codereview.appspot.com/6099055/diff/1/loop-unroll.c#newcode215
loop-unroll.c:215: compute_weighted_branches(struct loop *loop)
Missing space. Similarly for other functions.
http://codereview.appspot.com/6099055/diff/1/loop-unroll.c#newcode255
loop-unroll.c:255: data exists, simply return the current NUNROLL
factor. */
same here.
http://codereview.appspot.com/6099055/diff/1/loop-unroll.c#newcode281
loop-unroll.c:281: while (loop_outer(outer))
Should the check start from the current loop? Or the handling of the
current loop should be peeled out -- max_unroll_factor =
branch_budget/weighted_num_branches
Here we are trying to find a hot enclosing path, so we start from the
next most enclosing outer loop and walk outwards.
http://codereview.appspot.com/6099055/diff/1/loop-unroll.c#newcode317
loop-unroll.c:317: return (PARAM_VALUE
(PARAM_UNROLL_OUTER_LOOP_BRANCH_BUDGET) -
The number of branches may be larger than budget -- leading to overflow
-- need to guard against it.
Yes, this is a bug. I verified that it didn't have any real affect on
spec or internal benchmarks, thankfully, because in most cases where
there are more branches than the budget we have hit the outermost
(fake) loop and weren't executing this code. In the few cases we were,
we decided for other reasons not to unroll.
http://codereview.appspot.com/6099055/diff/1/loop-unroll.c#newcode318
loop-unroll.c:318: weighted_outer_branches)/(weighted_num_branches - 1)
+ 1;
Should it continue walking up the loop tree and find the minmal unroll
factor?
Once we find a hot enough enclosing loop we stop, because the number
of iterations is large enough to assume that the nested branches will
train the predictor and also are relatively hot enough to make the
benefits of unrolling the innermost loop outweigh any negative impact
on the outer region's branch prediction.
New patch coming shortly.
Thanks,
Teresa
http://codereview.appspot.com/6099055/diff/1/loop-unroll.c#newcode747
loop-unroll.c:747: loop_has_FP_comp(loop)
missing space
http://codereview.appspot.com/6099055/diff/1/loop-unroll.c#newcode749
loop-unroll.c:749: desc-niter (unsigned) PARAM_VALUE
(PARAM_MIN_ITER_UNROLL_WITH_BRANCHES))
line too long.
http://codereview.appspot.com/6099055/diff/1/loop-unroll.c#newcode1057
loop-unroll.c:1057: loop_has_FP_comp(loop)
Refactor the check into a helper function?
http://codereview.appspot.com/6099055/diff/1/params.def
File params.def (right):
http://codereview.appspot.com/6099055/diff/1/params.def#newcode314
params.def:314:
Missing comment.
http://codereview.appspot.com/6099055/diff/1/params.def#newcode319
params.def:319:
missing comment.
http://codereview.appspot.com/6099055/
--
Teresa Johnson | Software Engineer | tejohn...@google.com | 408-460-2413
