[Mesa-dev] [PATCH 3/4] radeonsi: add fine derivate control

[Michel Dänzer]

On 22.07.2015 07:51, Dave Airlie wrote:
> From: Dave Airlie <airlied at redhat.com>
> 
> This adds support for fine derivatives and enables
> ARB_derivative_control on radeonsi.
> 
> (just fell out of my working out interpolation)

In addition to Marek's comments:


> diff --git a/src/gallium/drivers/radeonsi/si_shader.c b/src/gallium/drivers/radeonsi/si_shader.c
> index f23eaa4..c5d80f0 100644
> --- a/src/gallium/drivers/radeonsi/si_shader.c
> +++ b/src/gallium/drivers/radeonsi/si_shader.c
> @@ -2203,7 +2203,8 @@ static void si_llvm_emit_ddxy(
>  	LLVMTypeRef i32;
>  	unsigned swizzle[4];
>  	unsigned c;
> -
> +	int idx;
> +	unsigned mask;
>  	i32 = LLVMInt32TypeInContext(gallivm->context);
>  
>  	indices[0] = bld_base->uint_bld.zero;

Please keep the blank line between the declarations and the i32 assignment.


> @@ -2212,14 +2213,21 @@ static void si_llvm_emit_ddxy(
>  	store_ptr = LLVMBuildGEP(gallivm->builder, si_shader_ctx->ddxy_lds,
>  				 indices, 2, "");
>  
> +	if (opcode == TGSI_OPCODE_DDX_FINE)
> +		mask = 0xfffffffe;
> +	else if (opcode == TGSI_OPCODE_DDY_FINE)
> +		mask = 0xfffffffd;
> +	else
> +		mask = 0xfffffffc;
>  	indices[1] = LLVMBuildAnd(gallivm->builder, indices[1],
> -				  lp_build_const_int32(gallivm, 0xfffffffc), "");
> +				  lp_build_const_int32(gallivm, mask), "");

Some macros for the masks might make the code a bit more readable, e.g.:

#define TID_MASK_TOP_LEFT	0xfffffffc
#define TID_MASK_LEFT		0xfffffffe
#define TID_MASK_TOP            0xfffffffd

Marek is right that some comments might make the code even clearer, but
you don't have to do that in this change.


> +	idx = (opcode == TGSI_OPCODE_DDX || opcode == TGSI_OPCODE_DDX_FINE) ? 1 :2;

Missing space between :2.

Also, maybe using a different name for this variable might make it
clearer that it's the delta for the LDS index from the top/left pixel to
the bottom/right one. Not coming up with any particularly good ideas
right now, but maybe tid_delta?


>  	indices[1] = LLVMBuildAdd(gallivm->builder, indices[1],
>  				  lp_build_const_int32(gallivm,
> -						       opcode == TGSI_OPCODE_DDX ? 1 : 2),
> +						       idx),
>  				  "");

Looks like the lp_build_const_int32() call could be collapsed to a
single line.


Other than that, looks good to me.


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