On Sat, Jan 19, 2019 at 12:27 AM Bas Nieuwenhuizen <[email protected]> wrote:
> On Sat, Jan 19, 2019 at 12:17 AM Timothy Arceri <[email protected]> wrote: > > > > > > > > On 19/1/19 9:36 am, Bas Nieuwenhuizen wrote: > > > On Thu, Jan 10, 2019 at 6:59 AM Timothy Arceri <[email protected]> > > > wrote: > > >> > > >> This builds on the recent interpolate fix by Rhys ee8488ea3b99. > > >> > > >> This doesn't handle arrays of structs but I've got a feeling those > > >> might be broken even for radeonsi tgsi (we currently have no tests). > > >> > > >> This fixes the arb_gpu_shader5 interpolateAt* tests that contain > > >> arrays. > > >> > > >> Fixes: ee8488ea3b99 ("ac/nir,radv,radeonsi/nir: use correct indices for > > >> interpolation intrinsics") > > >> --- > > >> src/amd/common/ac_nir_to_llvm.c | 80 +++++++++++++++++++++++++-------- > > >> 1 file changed, 61 insertions(+), 19 deletions(-) > > >> > > >> diff --git a/src/amd/common/ac_nir_to_llvm.c > > >> b/src/amd/common/ac_nir_to_llvm.c > > >> index 5023b96f92..00011a439d 100644 > > >> --- a/src/amd/common/ac_nir_to_llvm.c > > >> +++ b/src/amd/common/ac_nir_to_llvm.c > > >> @@ -2830,15 +2830,16 @@ static LLVMValueRef visit_interp(struct > > >> ac_nir_context *ctx, > > >> const nir_intrinsic_instr *instr) > > >> { > > >> LLVMValueRef result[4]; > > >> - LLVMValueRef interp_param, attr_number; > > >> + LLVMValueRef interp_param; > > >> unsigned location; > > >> unsigned chan; > > >> LLVMValueRef src_c0 = NULL; > > >> LLVMValueRef src_c1 = NULL; > > >> LLVMValueRef src0 = NULL; > > >> > > >> - nir_variable *var = > > >> nir_deref_instr_get_variable(nir_instr_as_deref(instr->src[0].ssa->parent_instr)); > > >> - int input_index = > > >> ctx->abi->fs_input_attr_indices[var->data.location - VARYING_SLOT_VAR0]; > > >> + nir_deref_instr *deref_instr = > > >> nir_instr_as_deref(instr->src[0].ssa->parent_instr); > > >> + nir_variable *var = nir_deref_instr_get_variable(deref_instr); > > >> + int input_base = > > >> ctx->abi->fs_input_attr_indices[var->data.location - VARYING_SLOT_VAR0]; > > >> switch (instr->intrinsic) { > > >> case nir_intrinsic_interp_deref_at_centroid: > > >> location = INTERP_CENTROID; > > >> @@ -2868,7 +2869,6 @@ static LLVMValueRef visit_interp(struct > > >> ac_nir_context *ctx, > > >> src_c1 = LLVMBuildFSub(ctx->ac.builder, src_c1, > > >> halfval, ""); > > >> } > > >> interp_param = ctx->abi->lookup_interp_param(ctx->abi, > > >> var->data.interpolation, location); > > >> - attr_number = LLVMConstInt(ctx->ac.i32, input_index, false); > > >> > > >> if (location == INTERP_CENTER) { > > >> LLVMValueRef ij_out[2]; > > >> @@ -2906,26 +2906,68 @@ static LLVMValueRef visit_interp(struct > > >> ac_nir_context *ctx, > > >> > > >> } > > >> > > >> + LLVMValueRef array_idx = ctx->ac.i32_0; > > >> + while(deref_instr->deref_type != nir_deref_type_var) { > > >> + if (deref_instr->deref_type == nir_deref_type_array) { > > >> + unsigned array_size = > > >> glsl_get_aoa_size(deref_instr->type); > > >> + if (!array_size) > > >> + array_size = 1; > > >> + > > >> + LLVMValueRef offset; > > >> + nir_const_value *const_value = > > >> nir_src_as_const_value(deref_instr->arr.index); > > >> + if (const_value) { > > >> + offset = LLVMConstInt(ctx->ac.i32, > > >> array_size * const_value->u32[0], false); > > >> + } else { > > >> + LLVMValueRef indirect = get_src(ctx, > > >> deref_instr->arr.index); > > >> + > > >> + offset = LLVMBuildMul(ctx->ac.builder, > > >> indirect, > > >> + > > >> LLVMConstInt(ctx->ac.i32, array_size, false), ""); > > >> + } > > >> + > > >> + array_idx = LLVMBuildAdd(ctx->ac.builder, > > >> array_idx, offset, ""); > > >> + deref_instr = > > >> nir_src_as_deref(deref_instr->parent); > > >> + } else if (deref_instr->deref_type == > > >> nir_deref_type_struct) { > > >> + /* TODO: Probably need to do more here to > > >> support arrays of structs etc */ > > >> + deref_instr = > > >> nir_src_as_deref(deref_instr->parent); > > > > > > If we don't have confidence this works can we just have it go to the > > > unreachable below. IIRC spirv->nir also lowered struct inputs so I'm > > > not even sure we would encounter this. > > > > This will work for structs, just probably not for arrays of structs. We > > do need struct handling for radeonsi so I'd rather leave this as is. Actually, how does this work for structs? I find it suspicous we don't care about which member is taken? > > > > > > > > Otherwise, > > > > > > Reviewed-by: Bas Nieuwenhuizen <[email protected]> > > > > > >> + } else { > > >> + unreachable("Unsupported deref type"); > > >> + } > > >> + > > >> + } > > >> + > > >> + unsigned input_array_size = glsl_get_aoa_size(var->type); > > >> + if (!input_array_size) > > >> + input_array_size = 1; > > >> + > > >> for (chan = 0; chan < 4; chan++) { > > >> + LLVMValueRef gather = > > >> LLVMGetUndef(LLVMVectorType(ctx->ac.f32, input_array_size)); > > >> LLVMValueRef llvm_chan = LLVMConstInt(ctx->ac.i32, > > >> chan, false); > > >> > > >> - if (interp_param) { > > >> - interp_param = LLVMBuildBitCast(ctx->ac.builder, > > >> + for (unsigned idx = 0; idx < input_array_size; ++idx) { > > >> + LLVMValueRef v, attr_number; > > >> + > > >> + attr_number = LLVMConstInt(ctx->ac.i32, > > >> input_base + idx, false); > > >> + if (interp_param) { > > >> + interp_param = > > >> LLVMBuildBitCast(ctx->ac.builder, > > >> interp_param, > > >> ctx->ac.v2f32, ""); > > >> - LLVMValueRef i = LLVMBuildExtractElement( > > >> - ctx->ac.builder, interp_param, > > >> ctx->ac.i32_0, ""); > > >> - LLVMValueRef j = LLVMBuildExtractElement( > > >> - ctx->ac.builder, interp_param, > > >> ctx->ac.i32_1, ""); > > >> - > > >> - result[chan] = ac_build_fs_interp(&ctx->ac, > > >> - llvm_chan, > > >> attr_number, > > >> - > > >> ctx->abi->prim_mask, i, j); > > >> - } else { > > >> - result[chan] = ac_build_fs_interp_mov(&ctx->ac, > > >> - > > >> LLVMConstInt(ctx->ac.i32, 2, false), > > >> - llvm_chan, > > >> attr_number, > > >> - > > >> ctx->abi->prim_mask); > > >> + LLVMValueRef i = LLVMBuildExtractElement( > > >> + ctx->ac.builder, interp_param, > > >> ctx->ac.i32_0, ""); > > >> + LLVMValueRef j = LLVMBuildExtractElement( > > >> + ctx->ac.builder, interp_param, > > >> ctx->ac.i32_1, ""); > > >> + > > >> + v = ac_build_fs_interp(&ctx->ac, > > >> llvm_chan, attr_number, > > >> + > > >> ctx->abi->prim_mask, i, j); > > >> + } else { > > >> + v = ac_build_fs_interp_mov(&ctx->ac, > > >> LLVMConstInt(ctx->ac.i32, 2, false), > > >> + llvm_chan, > > >> attr_number, ctx->abi->prim_mask); > > >> + } > > >> + > > >> + gather = LLVMBuildInsertElement(ctx->ac.builder, > > >> gather, v, > > >> + > > >> LLVMConstInt(ctx->ac.i32, idx, false), ""); > > >> } > > >> + > > >> + result[chan] = LLVMBuildExtractElement(ctx->ac.builder, > > >> gather, array_idx, ""); > > >> + > > >> } > > >> return ac_build_varying_gather_values(&ctx->ac, result, > > >> instr->num_components, > > >> var->data.location_frac); > > >> -- > > >> 2.20.1 > > >> > > >> _______________________________________________ > > >> mesa-dev mailing list > > >> [email protected] > > >> https://lists.freedesktop.org/mailman/listinfo/mesa-dev _______________________________________________ mesa-dev mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/mesa-dev
