[Mesa-dev,0/9] Skip automatic execsize for instructions with a width of 4

Submitted by Iago Toral Quiroga on March 9, 2016, 8:36 a.m.

Details

Message ID 1457512602.3005.38.camel@saturno.local.igalia.com
State New
Headers show

Not browsing as part of any series.

Patch hide | download patch | download mbox

diff --git a/src/mesa/drivers/dri/i965/brw_eu_emit.c b/src/mesa/drivers/dri/i965/brw_eu_emit.c
index 6f11f59..625447f 100644
--- a/src/mesa/drivers/dri/i965/brw_eu_emit.c
+++ b/src/mesa/drivers/dri/i965/brw_eu_emit.c
@@ -203,6 +203,7 @@  brw_set_dest(struct brw_codegen *p, brw_inst *inst, struct brw_reg dest)
     * or 16 (SIMD16), as that's normally correct.  However, when dealing with
     * small registers, we automatically reduce it to match the register size.
     */
+   assert(dest.width != BRW_EXECUTE_4 || brw_inst_exec_size(devinfo, inst) == dest.width);
    if (dest.width < BRW_EXECUTE_8)
       brw_inst_set_exec_size(devinfo, inst, dest.width);
 }

Comments

On Wed, Mar 09, 2016 at 09:36:42AM +0100, Iago Toral wrote:
> On Wed, 2016-03-09 at 09:54 +0200, Pohjolainen, Topi wrote:
> > On Mon, Mar 07, 2016 at 10:48:49AM +0100, Samuel Iglesias Gons?lvez wrote:
> > > Hello,
> > > 
> > > There is only one patch from this series that has been reviewed (patch
> > > 1).
> > > 
> > > Our plans is to start sending patches for adding fp64 support to i965
> > > driver in the coming weeks but they depend on these patches.
> > > 
> > > Can someone take a look at them? ;)
> > > 
> > > Sam
> > > 
> > > 
> > > On Thu, 2015-12-17 at 14:44 +0100, Samuel Iglesias Gonsálvez wrote:
> > > > Hello,
> > > > 
> > > > This patch series is a updated version of the one Iago sent last
> > > > week [0] that includes patches for gen6 too, as suggested by Jason.
> > > > 
> > > > We checked the gen9 code paths that work with a horizontal width of 4
> > > > and we think there won't be any regression on gen9... but we don't
> > > > have any gen9 machine to run piglit with these patches. Can someone
> > > > check it?
> > 
> > I rebased it and ran it through the test system, gen9 seems to be fine, I
> > only got one regression, and that was on old g965:
> 
> Awesome! would it be possible to run that test in g695 with the attached
> change? If this is a regression caused by our code it should break at
> the assert introduced with it.
> 
> > /tmp/build_root/m64/lib/piglit/bin/ext_framebuffer_multisample-accuracy all_samples srgb depthstencil -auto -fbo
> > Pixels that should be unlit
> >   count = 236444
> >   RMS error = 0.025355
> > Pixels that should be totally lit
> >   count = 13308
> >   Perfect output
> > The error threshold for unlit and totally lit pixels test is 0.016650
> > Pixels that should be partially lit
> >   count = 12392
> >   RMS error = 0.273876
> > The error threshold for partially lit pixels is 0.333000
> > Samples = 0, Result = fail
> > 
> > 
> > But I'm not sure if this is caused by your patches.
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> 

> diff --git a/src/mesa/drivers/dri/i965/brw_eu_emit.c b/src/mesa/drivers/dri/i965/brw_eu_emit.c
> index 6f11f59..625447f 100644
> --- a/src/mesa/drivers/dri/i965/brw_eu_emit.c
> +++ b/src/mesa/drivers/dri/i965/brw_eu_emit.c
> @@ -203,6 +203,7 @@ brw_set_dest(struct brw_codegen *p, brw_inst *inst, struct brw_reg dest)
>      * or 16 (SIMD16), as that's normally correct.  However, when dealing with
>      * small registers, we automatically reduce it to match the register size.
>      */
> +   assert(dest.width != BRW_EXECUTE_4 || brw_inst_exec_size(devinfo, inst) == dest.width);
>     if (dest.width < BRW_EXECUTE_8)
>        brw_inst_set_exec_size(devinfo, inst, dest.width);
>  }

Hmm, on top of your series this looks:

   /* Generators should set a default exec_size of either 8 (SIMD4x2 or SIMD8)
    * or 16 (SIMD16), as that's normally correct.  However, when dealing with
    * small registers, we automatically reduce it to match the register size.
    *
    * In platforms that support fp64 we can emit instructions with a width of
    * 4 that need two SIMD8 registers and an exec_size of 8 or 16. In these
    * cases we need to make sure that these instructions have their exec sizes
    * set properly when they are emitted and we can't rely on this code to fix
    * it.
    */
   bool fix_exec_size;
   if (devinfo->gen >= 6)
      fix_exec_size = dest.width < BRW_EXECUTE_4;
   else
      fix_exec_size = dest.width < BRW_EXECUTE_8;

   if (fix_exec_size)
      brw_inst_set_exec_size(devinfo, inst, dest.width);

Do you want the assertion before or after fixing?
On Wed, 2016-03-09 at 10:53 +0200, Pohjolainen, Topi wrote:
> On Wed, Mar 09, 2016 at 09:36:42AM +0100, Iago Toral wrote:
> > On Wed, 2016-03-09 at 09:54 +0200, Pohjolainen, Topi wrote:
> > > On Mon, Mar 07, 2016 at 10:48:49AM +0100, Samuel Iglesias Gons?lvez wrote:
> > > > Hello,
> > > > 
> > > > There is only one patch from this series that has been reviewed (patch
> > > > 1).
> > > > 
> > > > Our plans is to start sending patches for adding fp64 support to i965
> > > > driver in the coming weeks but they depend on these patches.
> > > > 
> > > > Can someone take a look at them? ;)
> > > > 
> > > > Sam
> > > > 
> > > > 
> > > > On Thu, 2015-12-17 at 14:44 +0100, Samuel Iglesias Gonsálvez wrote:
> > > > > Hello,
> > > > > 
> > > > > This patch series is a updated version of the one Iago sent last
> > > > > week [0] that includes patches for gen6 too, as suggested by Jason.
> > > > > 
> > > > > We checked the gen9 code paths that work with a horizontal width of 4
> > > > > and we think there won't be any regression on gen9... but we don't
> > > > > have any gen9 machine to run piglit with these patches. Can someone
> > > > > check it?
> > > 
> > > I rebased it and ran it through the test system, gen9 seems to be fine, I
> > > only got one regression, and that was on old g965:
> > 
> > Awesome! would it be possible to run that test in g695 with the attached
> > change? If this is a regression caused by our code it should break at
> > the assert introduced with it.
> > 
> > > /tmp/build_root/m64/lib/piglit/bin/ext_framebuffer_multisample-accuracy all_samples srgb depthstencil -auto -fbo
> > > Pixels that should be unlit
> > >   count = 236444
> > >   RMS error = 0.025355
> > > Pixels that should be totally lit
> > >   count = 13308
> > >   Perfect output
> > > The error threshold for unlit and totally lit pixels test is 0.016650
> > > Pixels that should be partially lit
> > >   count = 12392
> > >   RMS error = 0.273876
> > > The error threshold for partially lit pixels is 0.333000
> > > Samples = 0, Result = fail
> > > 
> > > 
> > > But I'm not sure if this is caused by your patches.
> > > _______________________________________________
> > > mesa-dev mailing list
> > > mesa-dev@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> > 
> 
> > diff --git a/src/mesa/drivers/dri/i965/brw_eu_emit.c b/src/mesa/drivers/dri/i965/brw_eu_emit.c
> > index 6f11f59..625447f 100644
> > --- a/src/mesa/drivers/dri/i965/brw_eu_emit.c
> > +++ b/src/mesa/drivers/dri/i965/brw_eu_emit.c
> > @@ -203,6 +203,7 @@ brw_set_dest(struct brw_codegen *p, brw_inst *inst, struct brw_reg dest)
> >      * or 16 (SIMD16), as that's normally correct.  However, when dealing with
> >      * small registers, we automatically reduce it to match the register size.
> >      */
> > +   assert(dest.width != BRW_EXECUTE_4 || brw_inst_exec_size(devinfo, inst) == dest.width);
> >     if (dest.width < BRW_EXECUTE_8)
> >        brw_inst_set_exec_size(devinfo, inst, dest.width);
> >  }
> 
> Hmm, on top of your series this looks:
> 
>    /* Generators should set a default exec_size of either 8 (SIMD4x2 or SIMD8)
>     * or 16 (SIMD16), as that's normally correct.  However, when dealing with
>     * small registers, we automatically reduce it to match the register size.
>     *
>     * In platforms that support fp64 we can emit instructions with a width of
>     * 4 that need two SIMD8 registers and an exec_size of 8 or 16. In these
>     * cases we need to make sure that these instructions have their exec sizes
>     * set properly when they are emitted and we can't rely on this code to fix
>     * it.
>     */
>    bool fix_exec_size;
>    if (devinfo->gen >= 6)
>       fix_exec_size = dest.width < BRW_EXECUTE_4;
>    else
>       fix_exec_size = dest.width < BRW_EXECUTE_8;
> 
>    if (fix_exec_size)
>       brw_inst_set_exec_size(devinfo, inst, dest.width);
> 
> Do you want the assertion before or after fixing?
> 

Before, you can put it right after that comment. Thanks!

Iago
On Wed, Mar 09, 2016 at 10:03:08AM +0100, Iago Toral wrote:
> On Wed, 2016-03-09 at 10:53 +0200, Pohjolainen, Topi wrote:
> > On Wed, Mar 09, 2016 at 09:36:42AM +0100, Iago Toral wrote:
> > > On Wed, 2016-03-09 at 09:54 +0200, Pohjolainen, Topi wrote:
> > > > On Mon, Mar 07, 2016 at 10:48:49AM +0100, Samuel Iglesias Gons?lvez wrote:
> > > > > Hello,
> > > > > 
> > > > > There is only one patch from this series that has been reviewed (patch
> > > > > 1).
> > > > > 
> > > > > Our plans is to start sending patches for adding fp64 support to i965
> > > > > driver in the coming weeks but they depend on these patches.
> > > > > 
> > > > > Can someone take a look at them? ;)
> > > > > 
> > > > > Sam
> > > > > 
> > > > > 
> > > > > On Thu, 2015-12-17 at 14:44 +0100, Samuel Iglesias Gonsálvez wrote:
> > > > > > Hello,
> > > > > > 
> > > > > > This patch series is a updated version of the one Iago sent last
> > > > > > week [0] that includes patches for gen6 too, as suggested by Jason.
> > > > > > 
> > > > > > We checked the gen9 code paths that work with a horizontal width of 4
> > > > > > and we think there won't be any regression on gen9... but we don't
> > > > > > have any gen9 machine to run piglit with these patches. Can someone
> > > > > > check it?
> > > > 
> > > > I rebased it and ran it through the test system, gen9 seems to be fine, I
> > > > only got one regression, and that was on old g965:
> > > 
> > > Awesome! would it be possible to run that test in g695 with the attached
> > > change? If this is a regression caused by our code it should break at
> > > the assert introduced with it.
> > > 
> > > > /tmp/build_root/m64/lib/piglit/bin/ext_framebuffer_multisample-accuracy all_samples srgb depthstencil -auto -fbo
> > > > Pixels that should be unlit
> > > >   count = 236444
> > > >   RMS error = 0.025355
> > > > Pixels that should be totally lit
> > > >   count = 13308
> > > >   Perfect output
> > > > The error threshold for unlit and totally lit pixels test is 0.016650
> > > > Pixels that should be partially lit
> > > >   count = 12392
> > > >   RMS error = 0.273876
> > > > The error threshold for partially lit pixels is 0.333000
> > > > Samples = 0, Result = fail
> > > > 
> > > > 
> > > > But I'm not sure if this is caused by your patches.
> > > > _______________________________________________
> > > > mesa-dev mailing list
> > > > mesa-dev@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> > > 
> > 
> > > diff --git a/src/mesa/drivers/dri/i965/brw_eu_emit.c b/src/mesa/drivers/dri/i965/brw_eu_emit.c
> > > index 6f11f59..625447f 100644
> > > --- a/src/mesa/drivers/dri/i965/brw_eu_emit.c
> > > +++ b/src/mesa/drivers/dri/i965/brw_eu_emit.c
> > > @@ -203,6 +203,7 @@ brw_set_dest(struct brw_codegen *p, brw_inst *inst, struct brw_reg dest)
> > >      * or 16 (SIMD16), as that's normally correct.  However, when dealing with
> > >      * small registers, we automatically reduce it to match the register size.
> > >      */
> > > +   assert(dest.width != BRW_EXECUTE_4 || brw_inst_exec_size(devinfo, inst) == dest.width);
> > >     if (dest.width < BRW_EXECUTE_8)
> > >        brw_inst_set_exec_size(devinfo, inst, dest.width);
> > >  }
> > 
> > Hmm, on top of your series this looks:
> > 
> >    /* Generators should set a default exec_size of either 8 (SIMD4x2 or SIMD8)
> >     * or 16 (SIMD16), as that's normally correct.  However, when dealing with
> >     * small registers, we automatically reduce it to match the register size.
> >     *
> >     * In platforms that support fp64 we can emit instructions with a width of
> >     * 4 that need two SIMD8 registers and an exec_size of 8 or 16. In these
> >     * cases we need to make sure that these instructions have their exec sizes
> >     * set properly when they are emitted and we can't rely on this code to fix
> >     * it.
> >     */
> >    bool fix_exec_size;
> >    if (devinfo->gen >= 6)
> >       fix_exec_size = dest.width < BRW_EXECUTE_4;
> >    else
> >       fix_exec_size = dest.width < BRW_EXECUTE_8;
> > 
> >    if (fix_exec_size)
> >       brw_inst_set_exec_size(devinfo, inst, dest.width);
> > 
> > Do you want the assertion before or after fixing?
> > 
> 
> Before, you can put it right after that comment. Thanks!

That is what I thought. Hold on, I'll give it a spin.
On Wed, Mar 09, 2016 at 11:05:17AM +0200, Pohjolainen, Topi wrote:
> On Wed, Mar 09, 2016 at 10:03:08AM +0100, Iago Toral wrote:
> > On Wed, 2016-03-09 at 10:53 +0200, Pohjolainen, Topi wrote:
> > > On Wed, Mar 09, 2016 at 09:36:42AM +0100, Iago Toral wrote:
> > > > On Wed, 2016-03-09 at 09:54 +0200, Pohjolainen, Topi wrote:
> > > > > On Mon, Mar 07, 2016 at 10:48:49AM +0100, Samuel Iglesias Gons?lvez wrote:
> > > > > > Hello,
> > > > > > 
> > > > > > There is only one patch from this series that has been reviewed (patch
> > > > > > 1).
> > > > > > 
> > > > > > Our plans is to start sending patches for adding fp64 support to i965
> > > > > > driver in the coming weeks but they depend on these patches.
> > > > > > 
> > > > > > Can someone take a look at them? ;)
> > > > > > 
> > > > > > Sam
> > > > > > 
> > > > > > 
> > > > > > On Thu, 2015-12-17 at 14:44 +0100, Samuel Iglesias Gonsálvez wrote:
> > > > > > > Hello,
> > > > > > > 
> > > > > > > This patch series is a updated version of the one Iago sent last
> > > > > > > week [0] that includes patches for gen6 too, as suggested by Jason.
> > > > > > > 
> > > > > > > We checked the gen9 code paths that work with a horizontal width of 4
> > > > > > > and we think there won't be any regression on gen9... but we don't
> > > > > > > have any gen9 machine to run piglit with these patches. Can someone
> > > > > > > check it?
> > > > > 
> > > > > I rebased it and ran it through the test system, gen9 seems to be fine, I
> > > > > only got one regression, and that was on old g965:
> > > > 
> > > > Awesome! would it be possible to run that test in g695 with the attached
> > > > change? If this is a regression caused by our code it should break at
> > > > the assert introduced with it.
> > > > 
> > > > > /tmp/build_root/m64/lib/piglit/bin/ext_framebuffer_multisample-accuracy all_samples srgb depthstencil -auto -fbo
> > > > > Pixels that should be unlit
> > > > >   count = 236444
> > > > >   RMS error = 0.025355
> > > > > Pixels that should be totally lit
> > > > >   count = 13308
> > > > >   Perfect output
> > > > > The error threshold for unlit and totally lit pixels test is 0.016650
> > > > > Pixels that should be partially lit
> > > > >   count = 12392
> > > > >   RMS error = 0.273876
> > > > > The error threshold for partially lit pixels is 0.333000
> > > > > Samples = 0, Result = fail
> > > > > 
> > > > > 
> > > > > But I'm not sure if this is caused by your patches.
> > > > > _______________________________________________
> > > > > mesa-dev mailing list
> > > > > mesa-dev@lists.freedesktop.org
> > > > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> > > > 
> > > 
> > > > diff --git a/src/mesa/drivers/dri/i965/brw_eu_emit.c b/src/mesa/drivers/dri/i965/brw_eu_emit.c
> > > > index 6f11f59..625447f 100644
> > > > --- a/src/mesa/drivers/dri/i965/brw_eu_emit.c
> > > > +++ b/src/mesa/drivers/dri/i965/brw_eu_emit.c
> > > > @@ -203,6 +203,7 @@ brw_set_dest(struct brw_codegen *p, brw_inst *inst, struct brw_reg dest)
> > > >      * or 16 (SIMD16), as that's normally correct.  However, when dealing with
> > > >      * small registers, we automatically reduce it to match the register size.
> > > >      */
> > > > +   assert(dest.width != BRW_EXECUTE_4 || brw_inst_exec_size(devinfo, inst) == dest.width);
> > > >     if (dest.width < BRW_EXECUTE_8)
> > > >        brw_inst_set_exec_size(devinfo, inst, dest.width);
> > > >  }
> > > 
> > > Hmm, on top of your series this looks:
> > > 
> > >    /* Generators should set a default exec_size of either 8 (SIMD4x2 or SIMD8)
> > >     * or 16 (SIMD16), as that's normally correct.  However, when dealing with
> > >     * small registers, we automatically reduce it to match the register size.
> > >     *
> > >     * In platforms that support fp64 we can emit instructions with a width of
> > >     * 4 that need two SIMD8 registers and an exec_size of 8 or 16. In these
> > >     * cases we need to make sure that these instructions have their exec sizes
> > >     * set properly when they are emitted and we can't rely on this code to fix
> > >     * it.
> > >     */
> > >    bool fix_exec_size;
> > >    if (devinfo->gen >= 6)
> > >       fix_exec_size = dest.width < BRW_EXECUTE_4;
> > >    else
> > >       fix_exec_size = dest.width < BRW_EXECUTE_8;
> > > 
> > >    if (fix_exec_size)
> > >       brw_inst_set_exec_size(devinfo, inst, dest.width);
> > > 
> > > Do you want the assertion before or after fixing?
> > > 
> > 
> > Before, you can put it right after that comment. Thanks!
> 
> That is what I thought. Hold on, I'll give it a spin.

Okay, now the system got really mad, I have some 12000 regressions on
g45, ilk and g965.

And for the test discussed above we hit the assert:

/tmp/build_root/m64/lib/piglit/bin/ext_framebuffer_multisample-accuracy all_samples srgb depthstencil -auto -fbo
Standard Error

ext_framebuffer_multisample-accuracy: /mnt/space/jenkins/jobs/Leeroy/workspace_2/repos/mesa/src/mesa/drivers/dri/i965/brw_eu_emit.c:213: brw_set_dest: Assertion `dest.width != BRW_EXECUTE_4 || brw_inst_exec_size(devinfo, inst) == dest.width' failed.
On Wed, 2016-03-09 at 11:42 +0200, Pohjolainen, Topi wrote:
> On Wed, Mar 09, 2016 at 11:05:17AM +0200, Pohjolainen, Topi wrote:
> > On Wed, Mar 09, 2016 at 10:03:08AM +0100, Iago Toral wrote:
> > > On Wed, 2016-03-09 at 10:53 +0200, Pohjolainen, Topi wrote:
> > > > On Wed, Mar 09, 2016 at 09:36:42AM +0100, Iago Toral wrote:
> > > > > On Wed, 2016-03-09 at 09:54 +0200, Pohjolainen, Topi wrote:
> > > > > > On Mon, Mar 07, 2016 at 10:48:49AM +0100, Samuel Iglesias Gons?lvez wrote:
> > > > > > > Hello,
> > > > > > > 
> > > > > > > There is only one patch from this series that has been reviewed (patch
> > > > > > > 1).
> > > > > > > 
> > > > > > > Our plans is to start sending patches for adding fp64 support to i965
> > > > > > > driver in the coming weeks but they depend on these patches.
> > > > > > > 
> > > > > > > Can someone take a look at them? ;)
> > > > > > > 
> > > > > > > Sam
> > > > > > > 
> > > > > > > 
> > > > > > > On Thu, 2015-12-17 at 14:44 +0100, Samuel Iglesias Gonsálvez wrote:
> > > > > > > > Hello,
> > > > > > > > 
> > > > > > > > This patch series is a updated version of the one Iago sent last
> > > > > > > > week [0] that includes patches for gen6 too, as suggested by Jason.
> > > > > > > > 
> > > > > > > > We checked the gen9 code paths that work with a horizontal width of 4
> > > > > > > > and we think there won't be any regression on gen9... but we don't
> > > > > > > > have any gen9 machine to run piglit with these patches. Can someone
> > > > > > > > check it?
> > > > > > 
> > > > > > I rebased it and ran it through the test system, gen9 seems to be fine, I
> > > > > > only got one regression, and that was on old g965:
> > > > > 
> > > > > Awesome! would it be possible to run that test in g695 with the attached
> > > > > change? If this is a regression caused by our code it should break at
> > > > > the assert introduced with it.
> > > > > 
> > > > > > /tmp/build_root/m64/lib/piglit/bin/ext_framebuffer_multisample-accuracy all_samples srgb depthstencil -auto -fbo
> > > > > > Pixels that should be unlit
> > > > > >   count = 236444
> > > > > >   RMS error = 0.025355
> > > > > > Pixels that should be totally lit
> > > > > >   count = 13308
> > > > > >   Perfect output
> > > > > > The error threshold for unlit and totally lit pixels test is 0.016650
> > > > > > Pixels that should be partially lit
> > > > > >   count = 12392
> > > > > >   RMS error = 0.273876
> > > > > > The error threshold for partially lit pixels is 0.333000
> > > > > > Samples = 0, Result = fail
> > > > > > 
> > > > > > 
> > > > > > But I'm not sure if this is caused by your patches.
> > > > > > _______________________________________________
> > > > > > mesa-dev mailing list
> > > > > > mesa-dev@lists.freedesktop.org
> > > > > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> > > > > 
> > > > 
> > > > > diff --git a/src/mesa/drivers/dri/i965/brw_eu_emit.c b/src/mesa/drivers/dri/i965/brw_eu_emit.c
> > > > > index 6f11f59..625447f 100644
> > > > > --- a/src/mesa/drivers/dri/i965/brw_eu_emit.c
> > > > > +++ b/src/mesa/drivers/dri/i965/brw_eu_emit.c
> > > > > @@ -203,6 +203,7 @@ brw_set_dest(struct brw_codegen *p, brw_inst *inst, struct brw_reg dest)
> > > > >      * or 16 (SIMD16), as that's normally correct.  However, when dealing with
> > > > >      * small registers, we automatically reduce it to match the register size.
> > > > >      */
> > > > > +   assert(dest.width != BRW_EXECUTE_4 || brw_inst_exec_size(devinfo, inst) == dest.width);
> > > > >     if (dest.width < BRW_EXECUTE_8)
> > > > >        brw_inst_set_exec_size(devinfo, inst, dest.width);
> > > > >  }
> > > > 
> > > > Hmm, on top of your series this looks:
> > > > 
> > > >    /* Generators should set a default exec_size of either 8 (SIMD4x2 or SIMD8)
> > > >     * or 16 (SIMD16), as that's normally correct.  However, when dealing with
> > > >     * small registers, we automatically reduce it to match the register size.
> > > >     *
> > > >     * In platforms that support fp64 we can emit instructions with a width of
> > > >     * 4 that need two SIMD8 registers and an exec_size of 8 or 16. In these
> > > >     * cases we need to make sure that these instructions have their exec sizes
> > > >     * set properly when they are emitted and we can't rely on this code to fix
> > > >     * it.
> > > >     */
> > > >    bool fix_exec_size;
> > > >    if (devinfo->gen >= 6)
> > > >       fix_exec_size = dest.width < BRW_EXECUTE_4;
> > > >    else
> > > >       fix_exec_size = dest.width < BRW_EXECUTE_8;
> > > > 
> > > >    if (fix_exec_size)
> > > >       brw_inst_set_exec_size(devinfo, inst, dest.width);
> > > > 
> > > > Do you want the assertion before or after fixing?
> > > > 
> > > 
> > > Before, you can put it right after that comment. Thanks!
> > 
> > That is what I thought. Hold on, I'll give it a spin.
> 
> Okay, now the system got really mad, I have some 12000 regressions on
> g45, ilk and g965.

Oh right, that was my mistake, for a moment I forgot that these changes
are only for gen6+, gen < 6 are not affected (as you can see above, we
only change the original code in brw_set_dst for gens >= 6), so for gens
< 6 it is expected that you hit the assert I provided. Sorry for wasting
your time :(

The only way to check if this test is a real regression is to run it
with and without our series and see if it consistently fails and passes
respectively.

> And for the test discussed above we hit the assert:
> 
> /tmp/build_root/m64/lib/piglit/bin/ext_framebuffer_multisample-accuracy all_samples srgb depthstencil -auto -fbo
> Standard Error
> 
> ext_framebuffer_multisample-accuracy: /mnt/space/jenkins/jobs/Leeroy/workspace_2/repos/mesa/src/mesa/drivers/dri/i965/brw_eu_emit.c:213: brw_set_dest: Assertion `dest.width != BRW_EXECUTE_4 || brw_inst_exec_size(devinfo, inst) == dest.width' failed.
>
On Wed, Mar 09, 2016 at 11:16:41AM +0100, Iago Toral wrote:
> On Wed, 2016-03-09 at 11:42 +0200, Pohjolainen, Topi wrote:
> > On Wed, Mar 09, 2016 at 11:05:17AM +0200, Pohjolainen, Topi wrote:
> > > On Wed, Mar 09, 2016 at 10:03:08AM +0100, Iago Toral wrote:
> > > > On Wed, 2016-03-09 at 10:53 +0200, Pohjolainen, Topi wrote:
> > > > > On Wed, Mar 09, 2016 at 09:36:42AM +0100, Iago Toral wrote:
> > > > > > On Wed, 2016-03-09 at 09:54 +0200, Pohjolainen, Topi wrote:
> > > > > > > On Mon, Mar 07, 2016 at 10:48:49AM +0100, Samuel Iglesias Gons?lvez wrote:
> > > > > > > > Hello,
> > > > > > > > 
> > > > > > > > There is only one patch from this series that has been reviewed (patch
> > > > > > > > 1).
> > > > > > > > 
> > > > > > > > Our plans is to start sending patches for adding fp64 support to i965
> > > > > > > > driver in the coming weeks but they depend on these patches.
> > > > > > > > 
> > > > > > > > Can someone take a look at them? ;)
> > > > > > > > 
> > > > > > > > Sam
> > > > > > > > 
> > > > > > > > 
> > > > > > > > On Thu, 2015-12-17 at 14:44 +0100, Samuel Iglesias Gonsálvez wrote:
> > > > > > > > > Hello,
> > > > > > > > > 
> > > > > > > > > This patch series is a updated version of the one Iago sent last
> > > > > > > > > week [0] that includes patches for gen6 too, as suggested by Jason.
> > > > > > > > > 
> > > > > > > > > We checked the gen9 code paths that work with a horizontal width of 4
> > > > > > > > > and we think there won't be any regression on gen9... but we don't
> > > > > > > > > have any gen9 machine to run piglit with these patches. Can someone
> > > > > > > > > check it?
> > > > > > > 
> > > > > > > I rebased it and ran it through the test system, gen9 seems to be fine, I
> > > > > > > only got one regression, and that was on old g965:
> > > > > > 
> > > > > > Awesome! would it be possible to run that test in g695 with the attached
> > > > > > change? If this is a regression caused by our code it should break at
> > > > > > the assert introduced with it.
> > > > > > 
> > > > > > > /tmp/build_root/m64/lib/piglit/bin/ext_framebuffer_multisample-accuracy all_samples srgb depthstencil -auto -fbo
> > > > > > > Pixels that should be unlit
> > > > > > >   count = 236444
> > > > > > >   RMS error = 0.025355
> > > > > > > Pixels that should be totally lit
> > > > > > >   count = 13308
> > > > > > >   Perfect output
> > > > > > > The error threshold for unlit and totally lit pixels test is 0.016650
> > > > > > > Pixels that should be partially lit
> > > > > > >   count = 12392
> > > > > > >   RMS error = 0.273876
> > > > > > > The error threshold for partially lit pixels is 0.333000
> > > > > > > Samples = 0, Result = fail
> > > > > > > 
> > > > > > > 
> > > > > > > But I'm not sure if this is caused by your patches.
> > > > > > > _______________________________________________
> > > > > > > mesa-dev mailing list
> > > > > > > mesa-dev@lists.freedesktop.org
> > > > > > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> > > > > > 
> > > > > 
> > > > > > diff --git a/src/mesa/drivers/dri/i965/brw_eu_emit.c b/src/mesa/drivers/dri/i965/brw_eu_emit.c
> > > > > > index 6f11f59..625447f 100644
> > > > > > --- a/src/mesa/drivers/dri/i965/brw_eu_emit.c
> > > > > > +++ b/src/mesa/drivers/dri/i965/brw_eu_emit.c
> > > > > > @@ -203,6 +203,7 @@ brw_set_dest(struct brw_codegen *p, brw_inst *inst, struct brw_reg dest)
> > > > > >      * or 16 (SIMD16), as that's normally correct.  However, when dealing with
> > > > > >      * small registers, we automatically reduce it to match the register size.
> > > > > >      */
> > > > > > +   assert(dest.width != BRW_EXECUTE_4 || brw_inst_exec_size(devinfo, inst) == dest.width);
> > > > > >     if (dest.width < BRW_EXECUTE_8)
> > > > > >        brw_inst_set_exec_size(devinfo, inst, dest.width);
> > > > > >  }
> > > > > 
> > > > > Hmm, on top of your series this looks:
> > > > > 
> > > > >    /* Generators should set a default exec_size of either 8 (SIMD4x2 or SIMD8)
> > > > >     * or 16 (SIMD16), as that's normally correct.  However, when dealing with
> > > > >     * small registers, we automatically reduce it to match the register size.
> > > > >     *
> > > > >     * In platforms that support fp64 we can emit instructions with a width of
> > > > >     * 4 that need two SIMD8 registers and an exec_size of 8 or 16. In these
> > > > >     * cases we need to make sure that these instructions have their exec sizes
> > > > >     * set properly when they are emitted and we can't rely on this code to fix
> > > > >     * it.
> > > > >     */
> > > > >    bool fix_exec_size;
> > > > >    if (devinfo->gen >= 6)
> > > > >       fix_exec_size = dest.width < BRW_EXECUTE_4;
> > > > >    else
> > > > >       fix_exec_size = dest.width < BRW_EXECUTE_8;
> > > > > 
> > > > >    if (fix_exec_size)
> > > > >       brw_inst_set_exec_size(devinfo, inst, dest.width);
> > > > > 
> > > > > Do you want the assertion before or after fixing?
> > > > > 
> > > > 
> > > > Before, you can put it right after that comment. Thanks!
> > > 
> > > That is what I thought. Hold on, I'll give it a spin.
> > 
> > Okay, now the system got really mad, I have some 12000 regressions on
> > g45, ilk and g965.
> 
> Oh right, that was my mistake, for a moment I forgot that these changes
> are only for gen6+, gen < 6 are not affected (as you can see above, we
> only change the original code in brw_set_dst for gens >= 6), so for gens
> < 6 it is expected that you hit the assert I provided. Sorry for wasting
> your time :(
> 
> The only way to check if this test is a real regression is to run it
> with and without our series and see if it consistently fails and passes
> respectively.

I ran using the commit just before your series as HEAD, and I didn't get any
failures. Then I re-ran with your series and I got the same regression
on g965. Unfortunately I don't have that hardware myself and therefore
I can't debug it further.
On Wed, 2016-03-09 at 18:31 +0200, Pohjolainen, Topi wrote:
> On Wed, Mar 09, 2016 at 11:16:41AM +0100, Iago Toral wrote:
> > On Wed, 2016-03-09 at 11:42 +0200, Pohjolainen, Topi wrote:
> > > On Wed, Mar 09, 2016 at 11:05:17AM +0200, Pohjolainen, Topi wrote:
> > > > On Wed, Mar 09, 2016 at 10:03:08AM +0100, Iago Toral wrote:
> > > > > On Wed, 2016-03-09 at 10:53 +0200, Pohjolainen, Topi wrote:
> > > > > > On Wed, Mar 09, 2016 at 09:36:42AM +0100, Iago Toral wrote:
> > > > > > > On Wed, 2016-03-09 at 09:54 +0200, Pohjolainen, Topi wrote:
> > > > > > > > On Mon, Mar 07, 2016 at 10:48:49AM +0100, Samuel Iglesias Gons?lvez wrote:
> > > > > > > > > Hello,
> > > > > > > > > 
> > > > > > > > > There is only one patch from this series that has been reviewed (patch
> > > > > > > > > 1).
> > > > > > > > > 
> > > > > > > > > Our plans is to start sending patches for adding fp64 support to i965
> > > > > > > > > driver in the coming weeks but they depend on these patches.
> > > > > > > > > 
> > > > > > > > > Can someone take a look at them? ;)
> > > > > > > > > 
> > > > > > > > > Sam
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > On Thu, 2015-12-17 at 14:44 +0100, Samuel Iglesias Gonsálvez wrote:
> > > > > > > > > > Hello,
> > > > > > > > > > 
> > > > > > > > > > This patch series is a updated version of the one Iago sent last
> > > > > > > > > > week [0] that includes patches for gen6 too, as suggested by Jason.
> > > > > > > > > > 
> > > > > > > > > > We checked the gen9 code paths that work with a horizontal width of 4
> > > > > > > > > > and we think there won't be any regression on gen9... but we don't
> > > > > > > > > > have any gen9 machine to run piglit with these patches. Can someone
> > > > > > > > > > check it?
> > > > > > > > 
> > > > > > > > I rebased it and ran it through the test system, gen9 seems to be fine, I
> > > > > > > > only got one regression, and that was on old g965:
> > > > > > > 
> > > > > > > Awesome! would it be possible to run that test in g695 with the attached
> > > > > > > change? If this is a regression caused by our code it should break at
> > > > > > > the assert introduced with it.
> > > > > > > 
> > > > > > > > /tmp/build_root/m64/lib/piglit/bin/ext_framebuffer_multisample-accuracy all_samples srgb depthstencil -auto -fbo
> > > > > > > > Pixels that should be unlit
> > > > > > > >   count = 236444
> > > > > > > >   RMS error = 0.025355
> > > > > > > > Pixels that should be totally lit
> > > > > > > >   count = 13308
> > > > > > > >   Perfect output
> > > > > > > > The error threshold for unlit and totally lit pixels test is 0.016650
> > > > > > > > Pixels that should be partially lit
> > > > > > > >   count = 12392
> > > > > > > >   RMS error = 0.273876
> > > > > > > > The error threshold for partially lit pixels is 0.333000
> > > > > > > > Samples = 0, Result = fail
> > > > > > > > 
> > > > > > > > 
> > > > > > > > But I'm not sure if this is caused by your patches.
> > > > > > > > _______________________________________________
> > > > > > > > mesa-dev mailing list
> > > > > > > > mesa-dev@lists.freedesktop.org
> > > > > > > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> > > > > > > 
> > > > > > 
> > > > > > > diff --git a/src/mesa/drivers/dri/i965/brw_eu_emit.c b/src/mesa/drivers/dri/i965/brw_eu_emit.c
> > > > > > > index 6f11f59..625447f 100644
> > > > > > > --- a/src/mesa/drivers/dri/i965/brw_eu_emit.c
> > > > > > > +++ b/src/mesa/drivers/dri/i965/brw_eu_emit.c
> > > > > > > @@ -203,6 +203,7 @@ brw_set_dest(struct brw_codegen *p, brw_inst *inst, struct brw_reg dest)
> > > > > > >      * or 16 (SIMD16), as that's normally correct.  However, when dealing with
> > > > > > >      * small registers, we automatically reduce it to match the register size.
> > > > > > >      */
> > > > > > > +   assert(dest.width != BRW_EXECUTE_4 || brw_inst_exec_size(devinfo, inst) == dest.width);
> > > > > > >     if (dest.width < BRW_EXECUTE_8)
> > > > > > >        brw_inst_set_exec_size(devinfo, inst, dest.width);
> > > > > > >  }
> > > > > > 
> > > > > > Hmm, on top of your series this looks:
> > > > > > 
> > > > > >    /* Generators should set a default exec_size of either 8 (SIMD4x2 or SIMD8)
> > > > > >     * or 16 (SIMD16), as that's normally correct.  However, when dealing with
> > > > > >     * small registers, we automatically reduce it to match the register size.
> > > > > >     *
> > > > > >     * In platforms that support fp64 we can emit instructions with a width of
> > > > > >     * 4 that need two SIMD8 registers and an exec_size of 8 or 16. In these
> > > > > >     * cases we need to make sure that these instructions have their exec sizes
> > > > > >     * set properly when they are emitted and we can't rely on this code to fix
> > > > > >     * it.
> > > > > >     */
> > > > > >    bool fix_exec_size;
> > > > > >    if (devinfo->gen >= 6)
> > > > > >       fix_exec_size = dest.width < BRW_EXECUTE_4;
> > > > > >    else
> > > > > >       fix_exec_size = dest.width < BRW_EXECUTE_8;
> > > > > > 
> > > > > >    if (fix_exec_size)
> > > > > >       brw_inst_set_exec_size(devinfo, inst, dest.width);
> > > > > > 
> > > > > > Do you want the assertion before or after fixing?
> > > > > > 
> > > > > 
> > > > > Before, you can put it right after that comment. Thanks!
> > > > 
> > > > That is what I thought. Hold on, I'll give it a spin.
> > > 
> > > Okay, now the system got really mad, I have some 12000 regressions on
> > > g45, ilk and g965.
> > 
> > Oh right, that was my mistake, for a moment I forgot that these changes
> > are only for gen6+, gen < 6 are not affected (as you can see above, we
> > only change the original code in brw_set_dst for gens >= 6), so for gens
> > < 6 it is expected that you hit the assert I provided. Sorry for wasting
> > your time :(
> > 
> > The only way to check if this test is a real regression is to run it
> > with and without our series and see if it consistently fails and passes
> > respectively.
> 
> I ran using the commit just before your series as HEAD, and I didn't get any
> failures. Then I re-ran with your series and I got the same regression
> on g965. Unfortunately I don't have that hardware myself and therefore
> I can't debug it further.

Ok, that sounds like a proper regression then, thanks for confirming
this Topi. Unfortunately I don't have the hardware either :( I'll see if
I get lucky with some of the old hardware we have at the office, but I
think we have already tried to look for gen4 hardware in the past and
did not find anything we could use...

Iago