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(-) > >
signature.asc
Description: This is a digitally signed message part.
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
