On 27 September 2012 16:56, Eric Anholt <span dir="ltr"><<a href="mailto:eric@anholt.net" target="_blank">eric@anholt.net</a>></span> wrote:<br><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im">Paul Berry <<a href="mailto:stereotype441@gmail.com">stereotype441@gmail.com</a>> writes:<br>
<br>
> On 19 September 2012 13:28, Kenneth Graunke <<a href="mailto:kenneth@whitecape.org">kenneth@whitecape.org</a>> wrote:<br>
</div><div class="im">>> diff --git a/src/mesa/drivers/dri/i965/brw_fs_reg_allocate.cpp<br>
>> b/src/mesa/drivers/dri/i965/brw_fs_reg_allocate.cpp<br>
>> index e7f11ae..a819ae0 100644<br>
>> --- a/src/mesa/drivers/dri/i965/brw_fs_reg_allocate.cpp<br>
>> +++ b/src/mesa/drivers/dri/i965/brw_fs_reg_allocate.cpp<br>
>> @@ -317,11 +317,19 @@ fs_visitor::choose_spill_reg(struct ra_graph *g)<br>
>> for (unsigned int i = 0; i < 3; i++) {<br>
>> if (inst->src[i].file == GRF) {<br>
>> spill_costs[inst->src[i].reg] += loop_scale;<br>
>> +<br>
>> + if (inst->src[i].smear >= 0) {<br>
>> + no_spill[inst->src[i].reg] = true;<br>
>> + }<br>
>><br>
><br>
> Can we add a comment above the if statement to alert people to why we can't<br>
> spill these registers (and why it's ok that we can't)? Perhaps something<br>
> like: "Register spilling logic assumes full-width registers; smeared<br>
> registers have a width of 1 so if we try to spill them we'll generate<br>
> invalid assembly. This shouldn't be a problem because smeared registers<br>
> are only used as short-term temporaries when loading pull constants, so<br>
> spilling them is unlikely to reduce register pressure anyhow."<br>
><br>
> I guess I shouldn't give this patch my "Reviewed-by" since I originated it,<br>
> but assuming a comment is added, feel free to add:<br>
<br>
</div>Yeah, that makes me scared that our spilling choices are bogus. I've<br>
been worried about that -- on LM when I reduced the number of registers<br>
used in its spilling shader, I got more spills happening, on some things<br>
that looked like poor choices. I did re-review the code, and didn't<br>
find anything at the time.<br>
<br>
</blockquote></div><br><div>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.</div><div><br></div><div>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.</div>
<div><br></div><div>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.</div>
<div><br></div><div>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:</div>
<div><br></div><div> "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)</div>
<div><br></div><div>I'll try this later today and see if it has any benefit.</div>