On 24 July 2015 at 13:29, Michel Dänzer <mic...@daenzer.net> wrote:
> On 22.07.2015 07:51, Dave Airlie wrote:
>> From: Dave Airlie <airl...@redhat.com>
>>
>> This is part of ARB_gpu_shader5, and this passes
>> all the piglit tests currently available.
>>
>> Signed-off-by: Dave Airlie <airl...@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.
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
