[Mesa-dev] [PATCH 7/7] i965: Don't spill "smeared" registers.

[Paul Berry]

On 27 September 2012 16:56, Eric Anholt <eric at anholt.net> wrote:

> Paul Berry <stereotype441 at gmail.com> writes:
>
> > On 19 September 2012 13:28, Kenneth Graunke <kenneth at whitecape.org>
> wrote:
> >> diff --git a/src/mesa/drivers/dri/i965/brw_fs_reg_allocate.cpp
> >> b/src/mesa/drivers/dri/i965/brw_fs_reg_allocate.cpp
> >> index e7f11ae..a819ae0 100644
> >> --- a/src/mesa/drivers/dri/i965/brw_fs_reg_allocate.cpp
> >> +++ b/src/mesa/drivers/dri/i965/brw_fs_reg_allocate.cpp
> >> @@ -317,11 +317,19 @@ fs_visitor::choose_spill_reg(struct ra_graph *g)
> >>        for (unsigned int i = 0; i < 3; i++) {
> >>          if (inst->src[i].file == GRF) {
> >>             spill_costs[inst->src[i].reg] += loop_scale;
> >> +
> >> +            if (inst->src[i].smear >= 0) {
> >> +               no_spill[inst->src[i].reg] = true;
> >> +            }
> >>
> >
> > Can we add a comment above the if statement to alert people to why we
> can't
> > spill these registers (and why it's ok that we can't)?  Perhaps something
> > like: "Register spilling logic assumes full-width registers; smeared
> > registers have a width of 1 so if we try to spill them we'll generate
> > invalid assembly.  This shouldn't be a problem because smeared registers
> > are only used as short-term temporaries when loading pull constants, so
> > spilling them is unlikely to reduce register pressure anyhow."
> >
> > I guess I shouldn't give this patch my "Reviewed-by" since I originated
> it,
> > but assuming a comment is added, feel free to add:
>
> Yeah, that makes me scared that our spilling choices are bogus.  I've
> been worried about that -- on LM when I reduced the number of registers
> used in its spilling shader, I got more spills happening, on some things
> that looked like poor choices.  I did re-review the code, and didn't
> find anything at the time.
>
>
I am so glad you brought this up, because it led me to re-read through our
register spilling code and I think I see a big bug in it.

The problem is that a lot of registers don't contribute to register
pressure at all (because they are trivially allocatable)--these are the
nodes that register allocation was able to remove from the interference
graph before it gave up.  There's no benefit to spilling these registers,
since they're not in the part of the interference graph that's uncolorable.
 We'll just have to loop around, discover that the graph is still
uncolorable, and try again.

This explains why we were so frequently trying to spill these "smeared"
registers in the first place: their spilling cost is really low since they
are read and written exactly once.  It also may help explain why when we
have to spill, we wind up spilling so many registers: because we spill all
of the low-cost zero-benefit registers before we get around to finding the
few moderate-cost high-benefit registers that are actually worth spilling.

I think the fix should be pretty straightforward: just have
ra_get_best_spill_node() skip registers for which in_stack is GL_TRUE.  If
you look at the paper our register allocator is based on ("Retargetable
Graph-Coloring Register Allocation for Irregular Architectures") this is
what they suggest, albeit subtly.  From page 8 of that paper:

    "4. Spill is invoked if Simplify fails to remove all nodes in the
graph. It picks one of the *remaining* nodes to spill, and inserts a load
before each use of the variable, and a store after each definition."
(emphasis mine)

I'll try this later today and see if it has any benefit.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20120928/80af5a3d/attachment.html>