[Mesa-dev] [PATCH] ac/nir_to_llvm: fix interpolateAt* for arrays
Bas Nieuwenhuizen
bas at basnieuwenhuizen.nl
Fri Jan 18 23:29:16 UTC 2019
On Sat, Jan 19, 2019 at 12:27 AM Bas Nieuwenhuizen
<bas at basnieuwenhuizen.nl> wrote:
> On Sat, Jan 19, 2019 at 12:17 AM Timothy Arceri <tarceri at itsqueeze.com> wrote:
> >
> >
> >
> > On 19/1/19 9:36 am, Bas Nieuwenhuizen wrote:
> > > On Thu, Jan 10, 2019 at 6:59 AM Timothy Arceri <tarceri at itsqueeze.com> 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 <bas at basnieuwenhuizen.nl>
> > >
> > >> + } 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
> > >> mesa-dev at lists.freedesktop.org
> > >> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
More information about the mesa-dev
mailing list