[Mesa-dev] [PATCH 3/4] i965/fs_surface_builder: Only apply predicate to components that exist

[Jason Ekstrand]

On Tue, Sep 15, 2015 at 6:39 AM, Francisco Jerez <currojerez at riseup.net> wrote:
> Jason Ekstrand <jason at jlekstrand.net> writes:
>
>> In certain conditions, we have to do bounds-checking in the shader for
>> image_load_store.  The way this works for image loads is that we do the
>> load anyway and then emit a series of slecects, one per component, that
>
> Strictly speaking the load is predicated so it's not really done for out
> of bounds coordinates, with that sentence clarified or removed (and the
> typo fixed in "slecects") this patch is:

Fixed.  Sorry about that; I forgot that we predicated the loads.  When
I wrote that I thought it sounded odd.

Out of curiosity, why don't we just emit a series of MOV dst.x 0
before the predicated load?  It's the same number of instructions and
might be able to be scheduled better.  I guess doing the selects is
more SSA-like.
--Jason

> Reviewed-by: Francisco Jerez <currojerez at riseup.net>
>
>> gives us 0 or the loaded value depending on whether or not you're in
>> bounds.  However, we were hard-coding 4 components which may not be
>> correct.  Instead, we should be using size which is the number of
>> components read.
>>
>> Cc: Francisco Jerez <currojerez at riseup.net>
>
> Although this is unlikely to fix any preexisting bugs (because the
> result of the extra SELs would have been left unused and they would have
> been dead-code-eliminated later on anyway), it shouldn't hurt to CC this
> to mesa-stable too, if you like.
>
>> ---
>>  src/mesa/drivers/dri/i965/brw_fs_surface_builder.cpp | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/src/mesa/drivers/dri/i965/brw_fs_surface_builder.cpp b/src/mesa/drivers/dri/i965/brw_fs_surface_builder.cpp
>> index 727e8d1..88f22fa 100644
>> --- a/src/mesa/drivers/dri/i965/brw_fs_surface_builder.cpp
>> +++ b/src/mesa/drivers/dri/i965/brw_fs_surface_builder.cpp
>> @@ -905,7 +905,7 @@ namespace brw {
>>              tmp = emit_untyped_read(bld, image, laddr, 1, size, pred);
>>
>>              /* An out of bounds surface access should give zero as result. */
>> -            for (unsigned c = 0; c < 4; ++c)
>> +            for (unsigned c = 0; c < size; ++c)
>>                 set_predicate(pred, bld.SEL(offset(tmp, bld, c),
>>                                             offset(tmp, bld, c), fs_reg(0)));
>>           }
>> --
>> 2.5.0.400.gff86faf