[Mesa-dev] [PATCH] ac/nir_to_llvm: fix interpolateAt* for arrays

Timothy Arceri tarceri at itsqueeze.com
Fri Jan 18 23:37:24 UTC 2019


On 19/1/19 10:29 am, Bas Nieuwenhuizen wrote:
> 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?

Yeah your right. It seems the piglit tests are too simple and always use 
the first member.

I think I will fall through to the unreachable() as you suggested for 
now. Then I'll write some better tests before adding proper struct support.

Thanks for the review.

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