[Mesa-dev] [PATCH 0/4] i965: skip control-flow aware liveness analysis if we only have 1 block

[Francisco Jerez]

Iago Toral <itoral at igalia.com> writes:

> On Tue, 2015-10-13 at 15:17 +0300, Francisco Jerez wrote:
>> Iago Toral Quiroga <itoral at igalia.com> writes:
>> 
>> > This fixes the following test:
>> >
>> > [require]
>> > GL >= 3.3
>> > GLSL >= 3.30
>> > GL_ARB_shader_storage_buffer_object
>> >
>> > [fragment shader]
>> > #version 330
>> > #extension GL_ARB_shader_storage_buffer_object: require
>> >
>> > buffer SSBO {
>> >     mat4 sm4;
>> > };
>> >
>> >
>> > mat4 um4;
>> >
>> > void main() {
>> >     sm4 *= um4;
>> 
>> This is using the value of "um4", which is never assigned, so liveness
>> analysis will correctly extend its live interval up to the start of the
>> block.
>> 
>> The other problem here seems to be that the liveness analysis pass
>> doesn't consider scalar writes (like the ones emitted by the
>> combine_constants optimization pass and by emit_uniformize()) to fully
>> define the contents of a register, so it will incorrectly extend up the
>> live interval of registers defined using scalar writes even if all
>> components ever used in the shader have been defined individually.
>> Incidentally the use-def-chains-based implementation of liveness
>> analysis I'm working on will fix this issue easily.
>
> Great, thanks Curro! Once you make your change public I can verify that
> they fix this too.
>
> BTW, even if your changes fix this I wonder if my patch is still valid:
> control-flow analysis should not add anything relevant to liveness
> analysis if we only have one block, right?
>

Yeah, that should be okay.  I'm not strongly opposed to this change as
temporary hack-around for the meantime, but the comments seem somewhat
misleading.

Either way patches 1 and 3 (i.e. the indentation fixes) are:

Reviewed-by: Francisco Jerez <currojerez at riseup.net>

> Iago
>
>> >     sm4[0][0] = 0.0;
>> > }
>> >
>> > [test]
>> > link success
>> >
>> > where our liveness analysis works really bad because the control-flow part
>> > will expand register liveness to the end of only block we have (so every
>> > register will be marked alive until the end of the program). This artificially
>> > increases register pressure to a point in which we run out of registers.
>> > Of course, this is only a simple optimization for a trivial case, the
>> > underlying problem still exists and would manifest when we have more than
>> > one block, but it helps simple shaders such as the one in the example above
>> > without any effort. I guess the real fix would involve re-thinking parts of the
>> > liveness analysis algorithm...
>> >
>> > Jordan, there is another thing that we can improve for this shader that is
>> > specific to SSBOs: we emit the same ssbo load multiple times because we are
>> > playing it safe just in case there are writes in between. I think we can try to
>> > do better and not re-issue the same load if we don't have ssbo stores to
>> > the same address in between. I'll try to come up with a patch for this
>> > (hopefully we can do this inside lower_ubo_reference).
>> >
>> > The actual fix goes into patches 2 (FS) and 4 (VS). Patches 1,3 are just
>> > indentation fixes in the code around these.
>> >
>> > Iago Toral Quiroga (4):
>> >   i965/fs: Fix indentation in fs_live_variables::compute_start_end
>> >   i965/fs: skip control-flow aware liveness analysis if we only have 1
>> >     block
>> >   i965/vec4: fix indentation in vec4_visitor::calculate_live_intervals
>> >   i965/vec4: skip control-flow aware liveness analysis if we only have 1
>> >     block
>> >
>> >  .../drivers/dri/i965/brw_fs_live_variables.cpp     | 24 ++++++++++++++--------
>> >  .../drivers/dri/i965/brw_vec4_live_variables.cpp   | 23 +++++++++++++--------
>> >  2 files changed, 30 insertions(+), 17 deletions(-)
>> >
>> > -- 
>> > 1.9.1
>> >
>> > _______________________________________________
>> > mesa-dev mailing list
>> > mesa-dev at lists.freedesktop.org
>> > http://lists.freedesktop.org/mailman/listinfo/mesa-dev
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 212 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20151013/09b100bb/attachment.sig>