[Mesa-dev] [PATCH 00/10] i965: always mark used surfaces in the visitors

[Iago Toral]

On Fri, 2015-10-30 at 16:19 +0200, Francisco Jerez wrote:
> Iago Toral Quiroga <itoral at igalia.com> writes:
> 
> > Right now some opcodes that only use constant surface indexing mark them as
> > used in the generator while others do it in the visitor. When the opcode can
> > handle both direct and indirect surface indexing then some opcodes handle
> > only the constant part in the generator and leave the indirect case to the
> > caller. It is all very inconsistent and leads to confusion, since one has to
> > go and look into the generator code in each case to check if it marks surfaces
> > as used or not, and in which cases.
> >
> > when I was working on SSBOs I was tempted to try and fix this but then I
> > forgot. Jordan bumped into this recently too when comparing visitor
> > code paths for similar opcodes (ubos and ssbos) that need to handle this
> > differently because they use different generator opcodes.
> >
> > Since the generator opcodes never handle marking of indirect surfaces, just
> > leave surface marking to the caller completely, since callers always have
> > all the information needed for this. It also makes things more consistent
> > and clear for everyone: marking surfaces as used is always on the side
> > of the visitor, never the generator.
> >
> > No piglit regressions observed in my IVB laptop. Would be nice to have
> > someone giving this a try with Jenkins though, to make sure I did not miss
> > anything in paths specific to other gens.
> 
> Jenkins seems to be mostly happy about the series except for three
> apparent regressions:
> 
>     piglit.spec.arb_fragment_layer_viewport.layer-gs-writes-in-range.bdwm64

Mmmm... not sure what could be going on with this, at first glance, it
does not look like this test should be affected by this series. The test
does not use any UBOs, does not really check what is written to the FBO,
does not emit texture accesses... Also, there are other tests in
piglit.spec.arb_fragment_layer_viewport that do very similar stuff
(specially the our-of-range test) and those are passing fine.

>     piglit.spec.arb_shader_texture_lod.compiler.tex_grad-texture2dproj-2d-vec4.frag.g965m64
>     piglit.spec.glsl-es-1_00.compiler.structure-and-array-operations.sampler-array-index.frag.g965m64
> 
> The latter two die with a crash so you may be able to look into them
> even if you don't have the original i965 by using INTEL_DEVID_OVERRIDE. ;)

Unfortunately it seems that these don't break for me with the DEVID
override, that's weird I guess, since they are compiler tests that I can
fully run without INTEL_NO_HW set:

$ INTEL_DEVID_OVERRIDE=0x29A2 bin/glslparsertest
generated_tests/spec/arb_shader_texture_lod/compiler/tex_grad-texture2DProj-2D-vec4.frag pass 1.10 GL_ARB_shader_texture_lod
Successfully compiled fragment shader
generated_tests/spec/arb_shader_texture_lod/compiler/tex_grad-texture2DProj-2D-vec4.frag: 
PIGLIT: {"result": "pass" }

$ INTEL_DEVID_OVERRIDE=0x29A2 bin/glslparsertest_gles2
tests/spec/glsl-es-1.00/compiler/structure-and-array-operations/sampler-array-index.frag pass 1.00
Successfully compiled fragment shader
tests/spec/glsl-es-1.00/compiler/structure-and-array-operations/sampler-array-index.frag: 0:21(21): warning: sampler arrays indexed with non-constant expressions will be forbidden in GLSL 3.00 and later
PIGLIT: {"result": "pass" }

I tried with most chipsets in between PCI_CHIP_I965_G (0x29A2) and
PCI_CHIP_G41_G (0x2E32).

Iago

> >
> > Iago Toral Quiroga (10):
> >   i965/fs: Do not mark direct used surfaces in
> >     VARYING_PULL_CONSTANT_LOAD
> >   i965/fs: Do not mark used direct surfaces in
> >     UNIFORM_PULL_CONSTANT_LOAD
> >   i965/fs: Do not mark used direct surfaces in the generator for texture
> >     opcodes
> >   i965/vec4: Do not mark used direct surfaces in
> >     VS_OPCODE_PULL_CONSTANT_LOAD
> >   i965/vec4: Do not mark used direct surfaces in the generator for
> >     texture opcodes
> >   i965/vec4: Do not mark used surfaces in SHADER_OPCODE_SHADER_TIME_ADD
> >   i965/fs: Do not mark used surfaces in SHADER_OPCODE_SHADER_TIME_ADD
> >   i965/vec4: Do not mark used surfaces in VS_OPCODE_GET_BUFFER_SIZE
> >   i965/fs: Do not mark used surfaces in FS_OPCODE_GET_BUFFER_SIZE
> >   i965/fs: Do not mark used surfaces in
> >     FS_OPCODE_FB_WRITE/FS_OPCODE_REP_FB_WRITE
> >
> >  src/mesa/drivers/dri/i965/brw_fs.cpp             | 12 ++++++-
> >  src/mesa/drivers/dri/i965/brw_fs.h               |  3 +-
> >  src/mesa/drivers/dri/i965/brw_fs_generator.cpp   | 31 -----------------
> >  src/mesa/drivers/dri/i965/brw_fs_nir.cpp         | 35 +++++++++++++------
> >  src/mesa/drivers/dri/i965/brw_fs_visitor.cpp     | 24 ++++++++-----
> >  src/mesa/drivers/dri/i965/brw_vec4.cpp           |  3 ++
> >  src/mesa/drivers/dri/i965/brw_vec4_generator.cpp | 19 ----------
> >  src/mesa/drivers/dri/i965/brw_vec4_nir.cpp       | 44 +++++++++++++++---------
> >  src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp   |  7 ++--
> >  9 files changed, 87 insertions(+), 91 deletions(-)
> >
> > -- 
> > 1.9.1
> >
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/mesa-dev