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

[Iago Toral]

On Wed, 2015-09-16 at 12:32 -0700, Kenneth Graunke wrote:
> 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).

It could be, I will have a look at one of the affected tests and try to
understand what is going on when we hit that case.

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

Yeah, adding them to brw_eu_emit.c looks like the best choice, I'll do
that and also add asserts to the generator.

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

Great, I'll work on the patches and send them for review. Thanks for the
feedback!

Iago

> > 
> > 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(-)
> > 
> >