[Mesa-dev] [PATCH 2/2] i965: Make brw_reg_from_fs_reg() halve exec_size when compressed.

[Kenneth Graunke]

On Tuesday, May 17, 2016 12:48:08 PM PDT Francisco Jerez wrote:
> Kenneth Graunke <kenneth at whitecape.org> writes:
> 
> > In a5d7e144eaf43fee37e6ff9e2de194407087632b, Connor generalized the
> > exec_size halving code to handle more cases.  As part of this, he made
> > it not halve anything if the region accessed falls completely in a
> > single register.
> >
> > Unfortunately, it started producing some invalid regions:
> >
> > -add(16)  g6<1>F  g10<8,8,1>UW    -g1<0,1,0>F    { align1 compr };
> > -add(16)  g8<1>F  g12<8,8,1>UW    -g1.1<0,1,0>F  { align1 compr };
> > +add(16)  g6<1>F  g10<16,16,1>UW  -g1<0,1,0>F    { align1 compr };
> > +add(16)  g8<1>F  g12<16,16,1>UW  -g1.1<0,1,0>F  { align1 compr };
> >
> > Here, the UW source region completely fits within a register.  However,
> > we have to use instruction compression because the destination region
> > spans two registers.  <16,16,1> is invalid because it's compressed.
> >
> > To handle this, skip the "everything fits in one register" case and
> > fall through to the exec_size halving case when compressed.
> >
> > Fixes hundreds of Piglit regressions on GM965.
> >
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=95370
> > Signed-off-by: Kenneth Graunke <kenneth at whitecape.org>
> > Cc: Connor Abbott <cwabbott0 at gmail.com>
> > Cc: Francisco Jerez <currojerez at riseup.net>
> > Cc: Matt Turner <mattst88 at gmail.com>
> > ---
> >  src/mesa/drivers/dri/i965/brw_fs_generator.cpp | 10 ++++++----
> >  1 file changed, 6 insertions(+), 4 deletions(-)
> >
> > diff --git a/src/mesa/drivers/dri/i965/brw_fs_generator.cpp b/src/mesa/
drivers/dri/i965/brw_fs_generator.cpp
> > index 4b85aaa..b9000d6 100644
> > --- a/src/mesa/drivers/dri/i965/brw_fs_generator.cpp
> > +++ b/src/mesa/drivers/dri/i965/brw_fs_generator.cpp
> > @@ -54,7 +54,8 @@ brw_file_from_reg(fs_reg *reg)
> >  }
> >  
> >  static struct brw_reg
> > -brw_reg_from_fs_reg(fs_inst *inst, fs_reg *reg, unsigned gen)
> > +brw_reg_from_fs_reg(const struct brw_codegen *p,
> > +                    fs_inst *inst, fs_reg *reg, unsigned gen)
> >  {
> >     struct brw_reg brw_reg;
> >  
> > @@ -65,7 +66,8 @@ brw_reg_from_fs_reg(fs_inst *inst, fs_reg *reg, unsigned 
gen)
> >     case VGRF:
> >        if (reg->stride == 0) {
> >           brw_reg = brw_vec1_reg(brw_file_from_reg(reg), reg->nr, 0);
> > -      } else if (inst->exec_size * reg->stride * type_sz(reg->type) <= 
32) {
> > +      } else if (!p->compressed &&
> 
> One of the things I had to fix for SIMD32 is a bunch of places that use
> p->compressed to find out whether the current execution size is 16 or 8,
> which just cannot represent exec_size=32 and seems kind of an abuse of
> the compressed flag because none of those cases actually care about
> compression or even emit compressed instructions AFAICT (they end up
> emitting either control flow instructions or send messages which are
> both uncompressed regardless of the execution size...).  Because I had
> to remove the only few uses left of compressed I also went ahead and got
> rid of the flag and the whole compressed_stack.  I doubt it's worth
> keeping just to pass the result of the compression control computation
> to this function.  Wouldn't a plain boolean flag or predicate function
> (e.g. taking inst as argument and saying whether it needs to be markede
> compressed) work for you?  If it does it will likely save me some
> clean-up work in the short term.

Yeah, that makes sense.  I've pushed these patches with Matt's review
so that G965 doesn't remain broken longer than it has to, but I've also
written a follow-on to not use p->compressed that I think you'll like
better.  I'll send it out shortly.
-------------- 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: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160517/ef80639e/attachment.sig>