Iago Toral <ito...@igalia.com> writes:
> On Tue, 2015-11-03 at 15:28 +0200, Francisco Jerez wrote:
>> Iago Toral <ito...@igalia.com> writes:
>>
>> > On Fri, 2015-10-30 at 16:19 +0200, Francisco Jerez wrote:
>> >> Iago Toral Quiroga <ito...@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 :-/
>
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@lists.freedesktop.org >> >> > http://lists.freedesktop.org/mailman/listinfo/mesa-dev
signature.asc
Description: PGP signature
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
