On Wed, Apr 11, 2018 at 4:25 PM, Juan A. Suarez Romero
<jasua...@igalia.com> wrote:
> Hi, Bas.
>
> Unfortunately, I can't apply this patch neither in next 17.3 release stable, 
> nor
> in 18.0, as this patch does not apply cleanly, and it requires other different
> commits that didn't land in the stable branches.

This backport (https://patchwork.freedesktop.org/patch/214949/)
applies cleanly on 17.3 for me.

>
>
> For 17.3 I think it is probably not worth to try to provide a specific version
> for the stable, as I'm already cooking the pre-release, and this is the latest
> release for 17.3 series.

Can we please get this in 17.3? This has been submitted upstream
laready for 2 weeks and this backport has been available for a week
and fixes one of the major games from Feral on Vega:

commit 4503ff760c794c3bb15b978a47c530037d56498e (dow3)
Author: Bas Nieuwenhuizen <b...@basnieuwenhuizen.nl>
Date:   Wed Mar 28 23:54:40 2018 +0200

Why did I not get a message that that commit could not be applied?

>
>
> Maybe it is worth to provide a version for 18.0 branch, though. Probably it
> wouldn't enter in this release, but surely in the next one which should happen
> in 1 or 2 weeks.
>
>
> For more information, for 18.0 this patch seems to require 1251f08ef ("ac: add
> ac_build_buffer_load_common() helper"), bac9fa9f17f ("ac: add glc parameter to
> ac_build_buffer_load_format"), and probably others.
>
>
>         J.A.
>
>
> On Wed, 2018-04-11 at 11:26 +0200, Bas Nieuwenhuizen wrote:
>> On Wed, Apr 4, 2018 at 10:19 PM, Bas Nieuwenhuizen
>> <b...@basnieuwenhuizen.nl> wrote:
>> > On GFX9 whether the buffer size is interpreted as elements or bytes
>> > depends on whether IDXEN is enabled in the instruction. If the index
>> > is a constant zero, LLVM optimizes IDXEN to 0.
>> >
>> > Now the size in elements is interpreted in bytes which of course
>> > results in out of bounds accesses.
>> >
>> > The correct fix is most likely to disable the LLVM optimization,
>> > but we need something to work with LLVM <= 6.0.
>> >
>> > radeonsi does the max between stride and element count on the CPU
>> > but that results in the size intrinsics returning the wrong size
>> > for the buffer. This would cause CTS errors for radv.
>> >
>> > v2: Also include the store changes.
>> >
>> > Fixes: e38685cc62e 'Revert "radv: disable support for VEGA for now."'
>> > (backported from 4503ff760c794c3bb15b978a47c530037d56498e for 17.3)
>> > ---
>> >  src/amd/common/ac_llvm_build.c  | 20 ++++++++++++++++++++
>> >  src/amd/common/ac_llvm_build.h  |  8 ++++++++
>> >  src/amd/common/ac_nir_to_llvm.c | 36 ++++++++++++++++++++++++++++++------
>> >  src/amd/common/ac_shader_abi.h  |  4 ++++
>> >  4 files changed, 62 insertions(+), 6 deletions(-)
>> >
>> > diff --git a/src/amd/common/ac_llvm_build.c 
>> > b/src/amd/common/ac_llvm_build.c
>> > index e5cd23e025..f193f71c3e 100644
>> > --- a/src/amd/common/ac_llvm_build.c
>> > +++ b/src/amd/common/ac_llvm_build.c
>> > @@ -960,6 +960,26 @@ LLVMValueRef ac_build_buffer_load_format(struct 
>> > ac_llvm_context *ctx,
>> >                                           AC_FUNC_ATTR_READONLY);
>> >  }
>> >
>> > +LLVMValueRef ac_build_buffer_load_format_gfx9_safe(struct ac_llvm_context 
>> > *ctx,
>> > +                                                  LLVMValueRef rsrc,
>> > +                                                  LLVMValueRef vindex,
>> > +                                                  LLVMValueRef voffset,
>> > +                                                  bool can_speculate)
>> > +{
>> > +       LLVMValueRef elem_count = LLVMBuildExtractElement(ctx->builder, 
>> > rsrc, LLVMConstInt(ctx->i32, 2, 0), "");
>> > +       LLVMValueRef stride = LLVMBuildExtractElement(ctx->builder, rsrc, 
>> > LLVMConstInt(ctx->i32, 1, 0), "");
>> > +       stride = LLVMBuildLShr(ctx->builder, stride, 
>> > LLVMConstInt(ctx->i32, 16, 0), "");
>> > +
>> > +       LLVMValueRef new_elem_count = LLVMBuildSelect(ctx->builder,
>> > +                                                     
>> > LLVMBuildICmp(ctx->builder, LLVMIntUGT, elem_count, stride, ""),
>> > +                                                     elem_count, stride, 
>> > "");
>> > +
>> > +       LLVMValueRef new_rsrc = LLVMBuildInsertElement(ctx->builder, rsrc, 
>> > new_elem_count,
>> > +                                                      
>> > LLVMConstInt(ctx->i32, 2, 0), "");
>> > +
>> > +       return ac_build_buffer_load_format(ctx, new_rsrc, vindex, voffset, 
>> > can_speculate);
>> > +}
>> > +
>> >  /**
>> >   * Set range metadata on an instruction.  This can only be used on load 
>> > and
>> >   * call instructions.  If you know an instruction can only produce the 
>> > values
>> > diff --git a/src/amd/common/ac_llvm_build.h 
>> > b/src/amd/common/ac_llvm_build.h
>> > index aa2a2899ab..d4264f2879 100644
>> > --- a/src/amd/common/ac_llvm_build.h
>> > +++ b/src/amd/common/ac_llvm_build.h
>> > @@ -188,6 +188,14 @@ LLVMValueRef ac_build_buffer_load_format(struct 
>> > ac_llvm_context *ctx,
>> >                                          LLVMValueRef voffset,
>> >                                          bool can_speculate);
>> >
>> > +/* load_format that handles the stride & element count better if idxen is
>> > + * disabled by LLVM. */
>> > +LLVMValueRef ac_build_buffer_load_format_gfx9_safe(struct ac_llvm_context 
>> > *ctx,
>> > +                                                  LLVMValueRef rsrc,
>> > +                                                  LLVMValueRef vindex,
>> > +                                                  LLVMValueRef voffset,
>> > +                                                  bool can_speculate);
>> > +
>> >  LLVMValueRef
>> >  ac_get_thread_id(struct ac_llvm_context *ctx);
>> >
>> > diff --git a/src/amd/common/ac_nir_to_llvm.c 
>> > b/src/amd/common/ac_nir_to_llvm.c
>> > index b696cb8a45..8ee80b1a67 100644
>> > --- a/src/amd/common/ac_nir_to_llvm.c
>> > +++ b/src/amd/common/ac_nir_to_llvm.c
>> > @@ -2257,11 +2257,19 @@ static LLVMValueRef build_tex_intrinsic(struct 
>> > ac_nir_context *ctx,
>> >                                         struct ac_image_args *args)
>> >  {
>> >         if (instr->sampler_dim == GLSL_SAMPLER_DIM_BUF) {
>> > -               return ac_build_buffer_load_format(&ctx->ac,
>> > -                                                  args->resource,
>> > -                                                  args->addr,
>> > -                                                  
>> > LLVMConstInt(ctx->ac.i32, 0, false),
>> > -                                                  true);
>> > +               if (ctx->abi->gfx9_stride_size_workaround) {
>> > +                       return 
>> > ac_build_buffer_load_format_gfx9_safe(&ctx->ac,
>> > +                                                                    
>> > args->resource,
>> > +                                                                    
>> > args->addr,
>> > +                                                                    
>> > ctx->ac.i32_0,
>> > +                                                                    true);
>> > +               } else {
>> > +                       return ac_build_buffer_load_format(&ctx->ac,
>> > +                                                          args->resource,
>> > +                                                          args->addr,
>> > +                                                          ctx->ac.i32_0,
>> > +                                                          true);
>> > +               }
>> >         }
>> >
>> >         args->opcode = ac_image_sample;
>> > @@ -3613,8 +3621,23 @@ static void visit_image_store(struct ac_nir_context 
>> > *ctx,
>> >                 glc = i1true;
>> >
>> >         if (glsl_get_sampler_dim(type) == GLSL_SAMPLER_DIM_BUF) {
>> > +               LLVMValueRef rsrc = get_sampler_desc(ctx, 
>> > instr->variables[0], AC_DESC_BUFFER, 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), "");
>> > +               }
>> > +
>> >                 params[0] = ac_to_float(&ctx->ac, get_src(ctx, 
>> > instr->src[2])); /* data */
>> > -               params[1] = get_sampler_desc(ctx, instr->variables[0], 
>> > AC_DESC_BUFFER, true, true);
>> > +               params[1] = rsrc;
>> >                 params[2] = LLVMBuildExtractElement(ctx->ac.builder, 
>> > get_src(ctx, instr->src[0]),
>> >                                                     ctx->ac.i32_0, ""); /* 
>> > vindex */
>> >                 params[3] = ctx->ac.i32_0; /* voffset */
>> > @@ -6645,6 +6668,7 @@ LLVMModuleRef 
>> > ac_translate_nir_to_llvm(LLVMTargetMachineRef tm,
>> >         ctx.abi.load_ssbo = radv_load_ssbo;
>> >         ctx.abi.load_sampler_desc = radv_get_sampler_desc;
>> >         ctx.abi.clamp_shadow_reference = false;
>> > +       ctx.abi.gfx9_stride_size_workaround = ctx.ac.chip_class == GFX9;
>> >
>> >         if (shader_count >= 2)
>> >                 ac_init_exec_full_mask(&ctx.ac);
>> > diff --git a/src/amd/common/ac_shader_abi.h 
>> > b/src/amd/common/ac_shader_abi.h
>> > index 14517d5570..b04dc076de 100644
>> > --- a/src/amd/common/ac_shader_abi.h
>> > +++ b/src/amd/common/ac_shader_abi.h
>> > @@ -92,6 +92,10 @@ struct ac_shader_abi {
>> >         /* Whether to clamp the shadow reference value to [0,1]on VI. 
>> > Radeonsi currently
>> >          * uses it due to promoting D16 to D32, but radv needs it off. */
>> >         bool clamp_shadow_reference;
>> > +
>> > +       /* Whether to workaround GFX9 ignoring the stride for the buffer 
>> > size if IDXEN=0
>> > +        * and LLVM optimizes an indexed load with constant index to 
>> > IDXEN=0. */
>> > +       bool gfx9_stride_size_workaround;
>> >  };
>> >
>> >  #endif /* AC_SHADER_ABI_H */
>> > --
>> > 2.16.2
>> >
>>
>> _______________________________________________
>> mesa-stable mailing list
>> mesa-sta...@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-stable
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Reply via email to