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(-) > > > > _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
