Iago Toral <ito...@igalia.com> writes: > On Tue, 2015-11-03 at 09:19 -0800, Mark Janes wrote: >> Francisco Jerez <curroje...@riseup.net> writes: >> >> > 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? >> >> 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? >
Sounds good to me.
> 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@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
