On 22.07.2015 07:51, Dave Airlie wrote: > From: Dave Airlie <airl...@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 _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
