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

[Timothy Arceri]

On 31/07/18 16:52, Bas Nieuwenhuizen wrote:
> 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);
> }

Right but that looks consistent with the dynamic index used in radeonsi 
see tex_fetch_ptrs()