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

[Michel Dänzer]

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.


> +        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.


> +	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?

Also, I wonder if we couldn't factor out some shared code between these
two functions.


> +		/* fetch sample ID, then fetch it's sample position */

Spelling: "its"


> +	LLVMTypeRef input_type = LLVMFloatTypeInContext(gallivm->context);
> +
> +	LLVMValueRef params = LLVMGetParam(si_shader_ctx->radeon_bld.main_fn, SI_PARAM_PRIM_MASK);

No blank line between declarations.


> +	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.


> +	if (inst->Instruction.Opcode == TGSI_OPCODE_INTERP_OFFSET ||
> +	    inst->Instruction.Opcode == TGSI_OPCODE_INTERP_SAMPLE) {
> +		LLVMValueRef ij_out[2];
> +
> +		LLVMValueRef ddxy_out = si_llvm_emit_ddxy_interp(bld_base, interp_param);
> +
> +		for (i = 0; i < 2; i++) {
> +			LLVMValueRef ix_ll = lp_build_const_int32(gallivm, i);
> +			LLVMValueRef iy_ll = lp_build_const_int32(gallivm, i + 2);
> +			LLVMValueRef ddx_el = LLVMBuildExtractElement(gallivm->builder,
> +								      ddxy_out, ix_ll, "");
> +			LLVMValueRef ddy_el = LLVMBuildExtractElement(gallivm->builder,
> +								      ddxy_out, iy_ll, "");
> +			LLVMValueRef interp_el = LLVMBuildExtractElement(gallivm->builder,
> +									 interp_param, ix_ll, "");
> +			LLVMValueRef temp1, temp2;
> +
> +			interp_el = LLVMBuildBitCast(gallivm->builder, interp_el,
> +						     LLVMFloatTypeInContext(gallivm->context), "");
> +
> +			temp1 = LLVMBuildFMul(gallivm->builder, ddx_el, emit_data->args[0], "");
> +
> +			temp1 = LLVMBuildFAdd(gallivm->builder, temp1, interp_el, "");
> +
> +			temp2 = LLVMBuildFMul(gallivm->builder, ddy_el, emit_data->args[1], "");
> +
> +			temp2 = LLVMBuildFAdd(gallivm->builder, temp2, temp1, "");
> +
> +			ij_out[i] = LLVMBuildBitCast(gallivm->builder,
> +						     temp2,
> +						     LLVMIntTypeInContext(gallivm->context, 32), "");
> +		}
> +		interp_param = lp_build_gather_values(bld_base->base.gallivm, ij_out, 2);
> +	}

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


Other than that, looks good to me.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer