[Mesa-dev] [PATCH 4/4] radeonsi: add support for interpolateAt functions

[Dave Airlie]

On 24 July 2015 at 13:29, Michel Dänzer <michel at daenzer.net> wrote:
> On 22.07.2015 07:51, Dave Airlie wrote:
>> From: Dave Airlie <airlied at redhat.com>
>>
>> This is part of ARB_gpu_shader5, and this passes
>> all the piglit tests currently available.
>>
>> Signed-off-by: Dave Airlie <airlied at redhat.com>
>
> [...]
>
>> @@ -2263,6 +2263,225 @@ static void si_llvm_emit_ddxy(
>>       emit_data->output[0] = lp_build_gather_values(gallivm, result, 4);
>>  }
>>
>> +/* return 4 values - v2i32 DDX, v2i32 DDY */
>> +static LLVMValueRef si_llvm_emit_ddxy_interp(
>> +     struct lp_build_tgsi_context * bld_base,
>> +     LLVMValueRef interp_ij)
>> +{
>> +     struct si_shader_context *si_shader_ctx = si_shader_context(bld_base);
>> +     struct gallivm_state *gallivm = bld_base->base.gallivm;
>> +     struct lp_build_context * base = &bld_base->base;
>
> Drop the space between the asterisk and "base". (I know you just copied
> and pasted that, but let's not spread it further).
>
> There are more instances of this in the patch.

Oh yes, consider them gone.

>
>
>> +        temp = LLVMBuildAnd(gallivm->builder, indices[1],
>> +                         lp_build_const_int32(gallivm, 0xfffffffe), "");
>> +
>> +     temp2 = LLVMBuildAnd(gallivm->builder, indices[1],
>> +                          lp_build_const_int32(gallivm, 0xfffffffd), "");
>
> Some inconsistent indentation here.
>
> If you define macros for the masks in the previous patch, please use
> those here.

fixed and done.

>> +     for (c = 0; c < 2; ++c) {
>> +             LLVMValueRef store_val;
>> +             LLVMValueRef c_ll = lp_build_const_int32(gallivm, c);
>> +
>> +             store_val = LLVMBuildExtractElement(gallivm->builder,
>> +                                                 interp_ij, c_ll, "");
>> +             LLVMBuildStore(gallivm->builder,
>> +                            store_val,
>> +                            store_ptr);
>
> Doesn't this need to take swizzles into account, similar to
> si_llvm_emit_ddxy?

No, since interp_ij is the I/J for the pixel, they aren't a set of channels,
Only later when we feed things to the interpolation instruction do the Src[0]
swizzles matter.

>
>
>> +     interp_param_idx = lookup_interp_param_index(shader->ps_input_interpolate[input_index],
>> +                                                  location);
>> +     if (interp_param_idx == -1)
>> +             return;
>
> Can this actually happen in the absence of a bug? If not, maybe add an
> assertion or something so developers can catch it.

The lookup_interp_param_index already has an fprintf in it.

> This code could probably use some comments to explain what it's doing.

I'll see what I can do, it's mostly ported from r600 with reference to
an nda interpolation document I had.

I can say my understanding of this code is pretty slim, it mostly
debugged itself into existence.

Dave.