[Mesa-dev] [PATCH v2 2/2] i965/fs: Handle non-const sample number in interpolateAtSample

[Neil Roberts]

Matt Turner <mattst88 at gmail.com> writes:

>> +static fs_reg
>> +get_num_samples_reg(fs_visitor *v)
>> +{
>> +   struct gl_program_parameter_list *params = v->prog->Parameters;
>> +   static gl_state_index tokens[STATE_LENGTH] = {
>
> I suspect this isn't thread-safe.

Do you mean because the tokens array is static? I don't think this is
written to so I was assuming it would be ok. I should have made it
const. Or did you mean something else?

>> +               bld.CMP(bld.null_reg_ud(),
>> +                       sample_src, i_reg,
>> +                       BRW_CONDITIONAL_EQ);
>
> I think you might be able to put all of this on one line.

Sadly it doesn't fit. I could probably make it two lines though.

> If you want, you could reverse the order of the loop (that is, iterate
> from num_samples_reg down to 0) and then save the
> opt_cmod_propagation() pass some work and just emit an ADD with a a
> conditional_mod in order to get rid of the CMP instruction.
>
>> +               inst = bld.emit(BRW_OPCODE_BREAK);
>> +               inst->predicate = BRW_PREDICATE_NORMAL;
>> +               bld.emit(BRW_OPCODE_WHILE);
>
> You can actually get rid of the BREAK by predicating the WHILE (and
> setting predicate_inverse on it). There's also a nice
> set_predicate(predicate, inst) function you should use.
>
> If you want to do that, I think it'll take a small amount of work in
> the BRW_OPCODE_WHILE case in brw_cfg.cpp to understand that a
> predicated WHILE has two successors.

That sounds like a good idea. I still have more to learn about the ISA
so I guess this is a good way to do it :)

>> +static bool
>> +find_interpolate_at_sample_in_block(nir_block *block,
>> +                                    void *data)
>> +{
>> +   nir_foreach_instr_safe(block, instr) {
>
> I don't think you need _safe here. At least I can't see why it's
> necessary.

Good point, I don't know why I put _safe there.

Thanks for looking at this.

Regards,
- Neil