[Mesa-dev] [RFC 0/3] i965: Enable up to 24 MRF registers in gen6

[Kenneth Graunke]

On Wednesday, September 16, 2015 11:17:53 AM Iago Toral Quiroga wrote:
> It seems that we have some bugs where we fail to compile shaders in gen6
> because we do not having enough MRF registers available (see bugs 86469 and
> 90631 for example). That triggered some discussion about the fact that SNB
> might actually have 24 MRF registers available, but since the docs where not
> very clear about this, it was suggested that it would be nice to try and
> experiment if that was the case.
> 
> These series of patches implement such test, basically they turn our fixed
> BRW_MAX_MRF into a macro that accepts the hardware generation and then changes
> the spilling code in brw_fs_reg_allocate.cpp to use MRF registers 21-23 for
> gen6 (something similar can be done for the vec4 code, I just did not do it
> yet).
> 
> The good news is that this seems to work fine, at least I can do a full piglit
> run without issues in SNB.

Sweet!

> In fact, this seems to help a lot of tests when I
> force spilling of everything in the FS backend (INTEL_DEBUG=spill_fs):
> 
> Using MRF registers 13-15 for spilling:
> crash: 5, fail 267, pass: 15853, skip: 11679, warn: 3
> 
> Using MRF registers 21-23 for spilling:
> crash: 5, fail 140, pass: 15980, skip: 11679, warn: 3
> 
> As you can see, we drop the fail ratio to almost 50%...

That seems odd - I wouldn't think using m13-15 vs. m21-23 would actually
make a difference.  Perhaps it's papering over a bug where we're failing
to notice that MRFs are in use?  If so, we should probably fix that (in
addition to making this change).

> The bad news is that, currently, we assert that MRF registers are within the
> supported range in brw_reg.h. This works fine now because the limit does not
> depend on the hardware generation, but these patches change that of course.
> The natural way to fix this would be to pass a generation argument to
> all brw_reg functions that can create a brw_reg, but I imagine that we don't
> want to do that only for this, right?

Yeah...it does seem a bit funny to add a generation parameter to brw_reg
functions just for an assert that the register number is in range.

What about adding the asserts in brw_set_src0 and brw_set_dest?  This
would catch BLORP and the Gen4 clip/sf/gs code that emits assembly
directly - it would catch everything.  But, unfortunately, at the last
minute...when it might be harder to debug.  So, I do like adding the
assertions to the generators as well.

> In that case, if we want to keep the
> asserts (I think we do) we need a way around that limitatation. The first
> patch in this series tries to move the asserts to the generator, but that won't
> manage things like blorp and other modules that can emit code directly, so we
> would lose the assert checks for those. Of course we could add individual
> asserts for these as needed, but it is not ideal. Alternatively, we could add
> a function wrapper to brw_message_reg that has the assert and use that
> version of the function from these places. In that case, this wrapper might not
> need to take in the generation number as parameter and could just check
> with 16 as the limit, since we really only use MRF registers
> beyond 16 for spilling, and we only handle spilling in code paths that end
> up going through the generator.
> 
> Or maybe we think this is just not worth it if it only helps gen6...

I'd like to do it.

> 
> what do you think? 
> 
> Iago Toral Quiroga (3):
>   i965: Move MRF register asserts to the generator
>   i965: Turn BRW_MAX_MRF into a macro that accepts a hardware generation
>   i965/fs: Use MRF registers 21-23 for spilling on gen6
> 
>  src/mesa/drivers/dri/i965/brw_eu_emit.c            |  2 +-
>  src/mesa/drivers/dri/i965/brw_fs.cpp               |  4 ++--
>  src/mesa/drivers/dri/i965/brw_fs_generator.cpp     | 14 +++++++----
>  src/mesa/drivers/dri/i965/brw_fs_reg_allocate.cpp  | 27 ++++++++++++----------
>  src/mesa/drivers/dri/i965/brw_ir_vec4.h            |  2 +-
>  src/mesa/drivers/dri/i965/brw_reg.h                |  5 +---
>  .../drivers/dri/i965/brw_schedule_instructions.cpp |  4 ++--
>  src/mesa/drivers/dri/i965/brw_vec4_generator.cpp   |  9 +++++---
>  8 files changed, 37 insertions(+), 30 deletions(-)
> 
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150916/fd8964a0/attachment.sig>