[Mesa-dev] [Mesa-stable] [PATCH] ac/nir: Make the GFX9 buffer size fix apply to image loads/atomics too.

[Bas Nieuwenhuizen]

On Mon, Apr 16, 2018 at 1:17 PM, Juan A. Suarez Romero
<jasuarez at igalia.com> wrote:
> On Mon, 2018-04-16 at 00:09 +0200, Bas Nieuwenhuizen wrote:
>> No clue how I missed those ...
>>
>> Fixes: 4503ff760c "ac/nir: Add workaround for GFX9 buffer views."
>> CC: <mesa-stable at lists.freedesktop.org>
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105320
>
>
> Hi, Bas!
>
> This commit is a candidate for next 17.3 and 18.0 stable releases.
>
>
> Regarding the next 17.3.9 release, which should be released today, as this is
> the last one of 17.3, I understand you want this patch as part of the release,
> right?

Its fine not to. The urgency isn't there compared to the original fix,
and of course being the last release also carries the additional risk
of not being able to fix it if the patch turns out to break something
unexpected. I did not expect it to make it in.

>
> If that's the case, I cherry-picked the commit and solved some trivial
> conflicts. You can check to verify I correctly solved them at:
>
> https://github.com/Igalia/release-mesa/commit/37ad9fc7c6cecdb8a99d071ca6fdc2d663
> 7501a8
>
>
> Related with that commit, maybe you want to ensure that this one also is
> correct:
>
> https://github.com/Igalia/release-mesa/commit/51b4bdc7761b30a56299ee80f51521151d
> 4eec47

Looks correct to me, but I don't see any diffs with the backport I provided?
>
>
>
> Regarding the next 18.0.1, is it fine if we post-pone this patch for next
> 18.0.2, or do you think this must be added in 18.0.1?

18.0.2 is fine for the same reason as above.

>
>
>         J.A.
>
>
>
>> ---
>>  src/amd/common/ac_nir_to_llvm.c | 39 +++++++++++++++++++--------------
>>  1 file changed, 22 insertions(+), 17 deletions(-)
>>
>> diff --git a/src/amd/common/ac_nir_to_llvm.c b/src/amd/common/ac_nir_to_llvm.c
>> index 3a3aa72988..45405d30fe 100644
>> --- a/src/amd/common/ac_nir_to_llvm.c
>> +++ b/src/amd/common/ac_nir_to_llvm.c
>> @@ -2191,6 +2191,25 @@ static LLVMValueRef get_image_coords(struct ac_nir_context *ctx,
>>       return res;
>>  }
>>
>> +static LLVMValueRef get_image_buffer_descriptor(struct ac_nir_context *ctx,
>> +                                                const nir_intrinsic_instr *instr, bool write)
>> +{
>> +     LLVMValueRef rsrc = get_sampler_desc(ctx, instr->variables[0], AC_DESC_BUFFER, NULL, true, write);
>> +     if (ctx->abi->gfx9_stride_size_workaround) {
>> +             LLVMValueRef elem_count = LLVMBuildExtractElement(ctx->ac.builder, rsrc, LLVMConstInt(ctx->ac.i32, 2, 0), "");
>> +             LLVMValueRef stride = LLVMBuildExtractElement(ctx->ac.builder, rsrc, LLVMConstInt(ctx->ac.i32, 1, 0), "");
>> +             stride = LLVMBuildLShr(ctx->ac.builder, stride, LLVMConstInt(ctx->ac.i32, 16, 0), "");
>> +
>> +             LLVMValueRef new_elem_count = LLVMBuildSelect(ctx->ac.builder,
>> +                                                           LLVMBuildICmp(ctx->ac.builder, LLVMIntUGT, elem_count, stride, ""),
>> +                                                           elem_count, stride, "");
>> +
>> +             rsrc = LLVMBuildInsertElement(ctx->ac.builder, rsrc, new_elem_count,
>> +                                           LLVMConstInt(ctx->ac.i32, 2, 0), "");
>> +     }
>> +     return rsrc;
>> +}
>> +
>>  static LLVMValueRef visit_image_load(struct ac_nir_context *ctx,
>>                                    const nir_intrinsic_instr *instr)
>>  {
>> @@ -2211,7 +2230,7 @@ static LLVMValueRef visit_image_load(struct ac_nir_context *ctx,
>>               unsigned num_channels = util_last_bit(mask);
>>               LLVMValueRef rsrc, vindex;
>>
>> -             rsrc = get_sampler_desc(ctx, instr->variables[0], AC_DESC_BUFFER, NULL, true, false);
>> +             rsrc = get_image_buffer_descriptor(ctx, instr, false);
>>               vindex = LLVMBuildExtractElement(ctx->ac.builder, get_src(ctx, instr->src[0]),
>>                                                ctx->ac.i32_0, "");
>>
>> @@ -2262,20 +2281,7 @@ static void visit_image_store(struct ac_nir_context *ctx,
>>               glc = ctx->ac.i1true;
>>
>>       if (dim == GLSL_SAMPLER_DIM_BUF) {
>> -             LLVMValueRef rsrc = get_sampler_desc(ctx, instr->variables[0], AC_DESC_BUFFER, NULL, true, true);
>> -
>> -             if (ctx->abi->gfx9_stride_size_workaround) {
>> -                     LLVMValueRef elem_count = LLVMBuildExtractElement(ctx->ac.builder, rsrc, LLVMConstInt(ctx->ac.i32, 2, 0), "");
>> -                     LLVMValueRef stride = LLVMBuildExtractElement(ctx->ac.builder, rsrc, LLVMConstInt(ctx->ac.i32, 1, 0), "");
>> -                     stride = LLVMBuildLShr(ctx->ac.builder, stride, LLVMConstInt(ctx->ac.i32, 16, 0), "");
>> -
>> -                     LLVMValueRef new_elem_count = LLVMBuildSelect(ctx->ac.builder,
>> -                                                                   LLVMBuildICmp(ctx->ac.builder, LLVMIntUGT, elem_count, stride, ""),
>> -                                                                   elem_count, stride, "");
>> -
>> -                     rsrc = LLVMBuildInsertElement(ctx->ac.builder, rsrc, new_elem_count,
>> -                                                   LLVMConstInt(ctx->ac.i32, 2, 0), "");
>> -             }
>> +             LLVMValueRef rsrc = get_image_buffer_descriptor(ctx, instr, true);
>>
>>               params[0] = ac_to_float(&ctx->ac, get_src(ctx, instr->src[2])); /* data */
>>               params[1] = rsrc;
>> @@ -2360,8 +2366,7 @@ static LLVMValueRef visit_image_atomic(struct ac_nir_context *ctx,
>>       params[param_count++] = get_src(ctx, instr->src[2]);
>>
>>       if (glsl_get_sampler_dim(type) == GLSL_SAMPLER_DIM_BUF) {
>> -             params[param_count++] = get_sampler_desc(ctx, instr->variables[0], AC_DESC_BUFFER,
>> -                                                      NULL, true, true);
>> +             params[param_count++] = get_image_buffer_descriptor(ctx, instr, true);
>>               params[param_count++] = LLVMBuildExtractElement(ctx->ac.builder, get_src(ctx, instr->src[0]),
>>                                                               ctx->ac.i32_0, ""); /* vindex */
>>               params[param_count++] = ctx->ac.i32_0; /* voffset */