[Mesa-dev] [PATCH 2/9] i965/vec4: Initialize LOD to 0.0f.

[Matt Turner]

On Tue, Oct 20, 2015 at 1:19 AM, Iago Toral <itoral at igalia.com> wrote:
> On Mon, 2015-10-19 at 23:11 -0700, Matt Turner wrote:
>> On Mon, Oct 19, 2015 at 9:09 PM, Matt Turner <mattst88 at gmail.com> wrote:
>> > We implement textureQueryLevels (which takes no arguments, save the
>> > sampler) using the resinfo message (which takes an argument of LOD).
>> > Without initializing it, we'd generate a MOV from the null register to
>> > load the LOD argument.
>> > ---
>> >  src/mesa/drivers/dri/i965/brw_vec4_nir.cpp | 2 +-
>> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/src/mesa/drivers/dri/i965/brw_vec4_nir.cpp b/src/mesa/drivers/dri/i965/brw_vec4_nir.cpp
>> > index ea1e3e7..c942c80 100644
>> > --- a/src/mesa/drivers/dri/i965/brw_vec4_nir.cpp
>> > +++ b/src/mesa/drivers/dri/i965/brw_vec4_nir.cpp
>> > @@ -1579,7 +1579,7 @@ vec4_visitor::nir_emit_texture(nir_tex_instr *instr)
>> >     const glsl_type *coord_type = NULL;
>> >     src_reg shadow_comparitor;
>> >     src_reg offset_value;
>> > -   src_reg lod, lod2;
>> > +   src_reg lod(0.0f), lod2;
>> >     src_reg sample_index;
>> >     src_reg mcs;
>>
>> The fs backend handles this differently --
>>
>>    if (op == ir_query_levels) {
>>       /* textureQueryLevels() is implemented in terms of TXS so we need to
>>        * pass a valid LOD argument.
>>        */
>>       assert(lod.file == BAD_FILE);
>>       lod = fs_reg(0u);
>>    }
>>
>> That kinda seems worse -- but it does indicate that I should probably
>> initialize lod with 0u instead, though it doesn't seem to matter...
>
>
> I don't care much either way, the good thing about the code in the fs
> backend is that it makes things more explicit. I think I kind of like
> that a bit better but works for me either way. Whatever we do, we should
> probably use the same solution in both backends though.

Agreed. I'll send a new patch that implements it using the fs way.

>> mov(8)          g10<1>F         [0F, 0F, 0F, 0F]VF
>> send(8)         g3<1>UW         g10<8,8,1>F
>>                  sampler resinfo SIMD8 Surface = 1 Sampler = 0 mlen 1 rlen 4
>>
>> Opinions?
>
> isn't LOD a float value by definition? Also, there are other parts of
> the FS code that initialize it to 0.0, like this (from
> lower_sampler_logical_send_gen7):
>
>    if (bld.shader->stage != MESA_SHADER_FRAGMENT &&
>        op == SHADER_OPCODE_TEX) {
>       op = SHADER_OPCODE_TXL;
>       lod = fs_reg(0.0f);
>    }

Interesting. I wonder if we should be doing that at a higher level (in
GLSL IR, for instance).

> In any case, in that same function we do:
>
>    for (unsigned i = 0; i < ARRAY_SIZE(sources); i++)
>       sources[i] = bld.vgrf(BRW_REGISTER_TYPE_F);
>
> and then later we do:
>
> bld.MOV(sources[length], lod)
>
> so even if we initialize lod to 0u it is going to end up as a float,
> which explains the result you obtained, so I think float is probably
> what we want.

Yep, that explains it. Thanks for tracking that down.