[Mesa-dev] [PATCH 00/10] i965: always mark used surfaces in the visitors
Iago Toral
itoral at igalia.com
Thu Nov 5 05:14:21 PST 2015
On Tue, 2015-11-03 at 09:19 -0800, Mark Janes wrote:
> Francisco Jerez <currojerez at riseup.net> writes:
>
> > Iago Toral <itoral at igalia.com> writes:
> >
> >> 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?
>
> This was fixed in https://bugs.freedesktop.org/show_bug.cgi?id=92744
>
> Ordinarily, all failures/fixes are tracked by commit, and failures due
> to an old branchpoint are filtered out. Unfortunately, these failures
> were intermittent.
>
> If you rebase, you shouldn't see those failures anymore.
Thanks Mark! It seems that we don't have regressions then.
Curro, I was thinking in pushing the patches you reviewed so far. I want
to look into the other patches too (the ones dealing with fb writes,
textures and shader time), but I got sidetracked with other stuff so
I'll probably do that later. Is this fine or would you rather postpone
pushing any of these until we got all these opcodes addressed?
Iago
> >>>
> >>> >> 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 :-/
> >>
> > Yeah, sounds like an unrelated intermittent failure. Mark, had you seen
> > this already?
> >
> >>> > 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
>
More information about the mesa-dev
mailing list