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

Tue Nov 3 07:25:11 PST 2015

On Tue, 2015-11-03 at 15:28 +0200, Francisco Jerez wrote:
> Iago Toral <itoral at igalia.com> writes:
> 
> > 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.
> >
> Odd, I guess it may have been an intermittent failing test that just
> happens to have failed during your run.  Mark, have you seen any of
> these fail intermittently by any chance?
> 
> >>     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" }
> >
> Looking at the logs it actually seems to have been an assertion failure:
> 
> glslparsertest_gles2:
> /mnt/space/jenkins/jobs/Leeroy/workspace/repos/mesa/src/mesa/drivers/dri/i965/brw_fs_live_variables.cpp:112:
> void brw::fs_live_variables::setup_one_write(brw::block_data*, fs_inst*,
> int, const fs_reg&): Assertion `var < num_vars' failed.
> 
> Did you test it on a debug build?

Yes, it was a debug build.

I also don't see how these patches could break that assertion. The
assert is related liveness analysis and the number of variables in the
shader code, but these patches do not change the code we emit (or even
the number of vgrfs we instantiate), they only move calls to
brw_mark_surface_used around, which has no relation with the shader code
at all :-/

> > 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