[Mesa-dev] [PATCH] ac: fix get_image_coords() for radeonsi

[Bas Nieuwenhuizen]

On Fri, Jul 27, 2018 at 11:40 AM, Timothy Arceri <tarceri at itsqueeze.com> wrote:
>
>
> On 27/07/18 19:10, Bas Nieuwenhuizen wrote:
>>
>> On Fri, Jul 27, 2018 at 7:32 AM, Timothy Arceri <tarceri at itsqueeze.com>
>> wrote:
>>>
>>> Because this was setting image to true we would end up calling
>>> si_load_image_desc() when we sould be calling
>>> si_load_sampler_desc().
>>
>>
>> Since the descriptor is part of an image, not a sampler,
>> get_image_descriptor looks like the right thing to me?
>>
>> What assertion are you getting?
>
>
> LLVMValueRef si_load_image_desc(struct si_shader_context *ctx,
>                                 LLVMValueRef list, LLVMValueRef index,
>                                 enum ac_descriptor_type desc_type, bool
> dcc_off)
> {
>         LLVMBuilderRef builder = ctx->ac.builder;
>         LLVMValueRef rsrc;
>
>         if (desc_type == AC_DESC_BUFFER) {
>                 index = LLVMBuildMul(builder, index,
>                                      LLVMConstInt(ctx->i32, 2, 0), "");
>                 index = LLVMBuildAdd(builder, index,
>                                      ctx->i32_1, "");
>                 list = LLVMBuildPointerCast(builder, list,
>
> ac_array_in_const32_addr_space(ctx->v4i32), "");
>         } else {
>                 assert(desc_type == AC_DESC_IMAGE);
>         }
>
>         rsrc = ac_build_load_to_sgpr(&ctx->ac, list, index);
>         if (desc_type == AC_DESC_IMAGE && dcc_off)
>                 rsrc = force_dcc_off(ctx, rsrc);
>         return rsrc;
> }
>
> vs
>
> LLVMValueRef si_load_sampler_desc(struct si_shader_context *ctx,
>                                   LLVMValueRef list, LLVMValueRef index,
>                                   enum ac_descriptor_type type)
> {
>         LLVMBuilderRef builder = ctx->ac.builder;
>
>         switch (type) {
>         case AC_DESC_IMAGE:
>                 /* The image is at [0:7]. */
>                 index = LLVMBuildMul(builder, index, LLVMConstInt(ctx->i32,
> 2, 0), "");
>                 break;
>         case AC_DESC_BUFFER:
>                 /* The buffer is in [4:7]. */
>                 index = LLVMBuildMul(builder, index, LLVMConstInt(ctx->i32,
> 4, 0), "");
>                 index = LLVMBuildAdd(builder, index, ctx->i32_1, "");
>                 list = LLVMBuildPointerCast(builder, list,
>
> ac_array_in_const32_addr_space(ctx->v4i32), "");
>                 break;
>         case AC_DESC_FMASK:
>                 /* The FMASK is at [8:15]. */
>                 index = LLVMBuildMul(builder, index, LLVMConstInt(ctx->i32,
> 2, 0), "");
>                 index = LLVMBuildAdd(builder, index, ctx->i32_1, "");
>                 break;
>         case AC_DESC_SAMPLER:
>                 /* The sampler state is at [12:15]. */
>                 index = LLVMBuildMul(builder, index, LLVMConstInt(ctx->i32,
> 4, 0), "");
>                 index = LLVMBuildAdd(builder, index, LLVMConstInt(ctx->i32,
> 3, 0), "");
>                 list = LLVMBuildPointerCast(builder, list,
>
> ac_array_in_const32_addr_space(ctx->v4i32), "");
>                 break;
>         }
>
>         return ac_build_load_to_sgpr(&ctx->ac, list, index);
>
> }

I think we should fix the image path to get an fmask descriptor
though. If you look one level up the bounding of dynamic indices is
different for textures and images:

static LLVMValueRef
si_nir_load_sampler_desc(struct ac_shader_abi *abi,
                 unsigned descriptor_set, unsigned base_index,
                 unsigned constant_index, LLVMValueRef dynamic_index,
                 enum ac_descriptor_type desc_type, bool image,
             bool write, bool bindless)
{
    struct si_shader_context *ctx = si_shader_context_from_abi(abi);
    LLVMBuilderRef builder = ctx->ac.builder;
    LLVMValueRef list = LLVMGetParam(ctx->main_fn,
ctx->param_samplers_and_images);
    LLVMValueRef index;

    assert(!descriptor_set);

    dynamic_index = dynamic_index ? dynamic_index : ctx->ac.i32_0;
    index = LLVMBuildAdd(builder, dynamic_index,
                 LLVMConstInt(ctx->ac.i32, base_index + constant_index, false),
                 "");

    if (image) {
        assert(desc_type == AC_DESC_IMAGE || desc_type == AC_DESC_BUFFER);
        assert(base_index + constant_index < ctx->num_images);

        if (dynamic_index)
            index = si_llvm_bound_index(ctx, index, ctx->num_images);

        index = LLVMBuildSub(ctx->ac.builder,
                     LLVMConstInt(ctx->i32, SI_NUM_IMAGES - 1, 0),
                     index, "");

        /* TODO: be smarter about when we use dcc_off */
        return si_load_image_desc(ctx, list, index, desc_type, write);
    }

    assert(base_index + constant_index < ctx->num_samplers);

    if (dynamic_index)
        index = si_llvm_bound_index(ctx, index, ctx->num_samplers);

    index = LLVMBuildAdd(ctx->ac.builder, index,
                 LLVMConstInt(ctx->i32, SI_NUM_IMAGES / 2, 0), "");

    return si_load_sampler_desc(ctx, list, index, desc_type);
}
>>
>>
>>>
>>> This fixes an assert() in Deus Ex: MD
>>> ---
>>>   src/amd/common/ac_nir_to_llvm.c | 3 ++-
>>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/src/amd/common/ac_nir_to_llvm.c
>>> b/src/amd/common/ac_nir_to_llvm.c
>>> index cffc980e51f..fa934e6702e 100644
>>> --- a/src/amd/common/ac_nir_to_llvm.c
>>> +++ b/src/amd/common/ac_nir_to_llvm.c
>>> @@ -2250,7 +2250,8 @@ static void get_image_coords(struct ac_nir_context
>>> *ctx,
>>>
>>> fmask_load_address[1],
>>>
>>> fmask_load_address[2],
>>>
>>> sample_index,
>>> -
>>> get_image_descriptor(ctx, instr, AC_DESC_FMASK, false));
>>> +
>>> get_sampler_desc(ctx, nir_instr_as_deref(instr->src[0].ssa->parent_instr),
>>> +
>>> AC_DESC_FMASK, NULL, false, false));
>>>          }
>>>          if (count == 1 && !gfx9_1d) {
>>>                  if (instr->src[1].ssa->num_components)
>>> --
>>> 2.17.1
>>>
>>> _______________________________________________
>>> mesa-dev mailing list
>>> mesa-dev at lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev