[Mesa-dev] [PATCH 5/5] llvmpipe: disable simple_shader optimization

[Jose Fonseca]

Roland,

Series looks good AFAICT. Thanks for nailing this nasty and long standing issues.

----- Original Message -----
> On Tue, May 21, 2013 at 6:12 PM,  <sroland at vmware.com> wrote:
> > From: Roland Scheidegger <sroland at vmware.com>
> >
> > This optimization disabled mask checks if the shader is simple enough.
> > While this should work correctly, the problem is that it can hide real
> > issues
> > because shaders in practice are usually complex enough (8 instructions or 1
> > texture is already enough) so this doesn't get used, whereas dumbed-down
> > tests which should hit all the same code paths suddenly do something quite
> > different. This was the reason that bug 41787 could not be easily tracked
> > as
> > stencil test not working correctly (piglit would in fact have failed some
> > tests without that optimization).
> > So disable it for now, it's unclear if it's much of a win in any case.
> > ---
> >  src/gallium/drivers/llvmpipe/lp_state_fs.c |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/src/gallium/drivers/llvmpipe/lp_state_fs.c
> > b/src/gallium/drivers/llvmpipe/lp_state_fs.c
> > index 9661273..b06f915 100644
> > --- a/src/gallium/drivers/llvmpipe/lp_state_fs.c
> > +++ b/src/gallium/drivers/llvmpipe/lp_state_fs.c
> > @@ -247,7 +247,7 @@ generate_fs_loop(struct gallivm_state *gallivm,
> >     struct lp_build_mask_context mask;
> >     boolean simple_shader =
> >     (shader->info.base.file_count[TGSI_FILE_SAMPLER] == 0 &&
> >                              shader->info.base.num_inputs < 3 &&
> > -                            shader->info.base.num_instructions < 8);
> > +                            shader->info.base.num_instructions < 8) && 0;
> 
> Maybe add a comment here as per your commit message as to why it's disabled.

Agreed.

You often write very thoughtful commit messages, but unfortunately for future reference commit messages can only be seen through git blame, and until any refactory completely hides them away.  It would be preferable to have the detailed explanation as comments, and merely say "See code comments for details" in the commit messages.

Jose

> 
> Alex
> 
> >     const boolean dual_source_blend = key->blend.rt[0].blend_enable &&
> >                                       util_blend_state_is_dual(&key->blend,
> >                                       0);
> >     unsigned attrib;
> > --
> > 1.7.9.5
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>