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

[Paul Berry]

On 19 September 2012 13:28, Kenneth Graunke <kenneth at whitecape.org> wrote:

> From: Paul Berry <stereotype441 at gmail.com>
>
> Fixes an assertion failure when compiling certain shaders that need both
> pull constants and register spilling:
>
> brw_eu_emit.c:204: validate_reg: Assertion `execsize >= width' failed.
>
> Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>
> ---
>  src/mesa/drivers/dri/i965/brw_fs_reg_allocate.cpp | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> Patch contents by Paul; commit message by me.


> 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:

Signed-off-by: Paul Berry <stereotype441 at gmail.com>


>          }
>        }
>
>        if (inst->dst.file == GRF) {
>          spill_costs[inst->dst.reg] += inst->regs_written() * loop_scale;
> +
> +         if (inst->dst.smear >= 0) {
> +            no_spill[inst->dst.reg] = true;
> +         }
>        }
>
>        switch (inst->opcode) {
> --
> 1.7.11.4
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20120919/42308c71/attachment-0001.html>