[Mesa-dev] [PATCH] spirv: Work around the Doom shader bug

[Pierre-Loup A. Griffais]

They gave us access to a build with the fix and I believe Dave confirmed 
the fix was looking good by running it against a mainline build of radv. 
I can't comment on what their planned timeline is for releasing it or 
whether it's going to be released for sure, but it seems that title is 
going to be in good shape eventually. What's the other title you saw it 
with?

On 06/20/2017 12:11 PM, Connor Abbott wrote:
> I've been told by Pierre that there is an update, but it just hasn't
> been pushed out yet. He'd know better when it's supposed to be
> released.
> 
> On Tue, Jun 20, 2017 at 9:11 AM, Jason Ekstrand <jason at jlekstrand.net> wrote:
>> Doom shipped with a broken version of GLSLang which handles samplers as
>> function arguments in a way that isn't spec-compliant.  In particular,
>> it creates a temporary local sampler variable and copies the sampler
>> into it.  While Dave has had a hack patch out for a while that gets it
>> working, we've never landed it because we've been hoping that a game
>> update would come out with fixed shaders.  Unfortunately, no game update
>> appears on to be on the horizon and I've found this issue in yet another
>> application so I think we're stuck working around it.  Hopefully, we can
>> delete this code one day.
>>
>> Cc: Dave Airlie <airlied at redhat.com>
>> ---
>>   src/compiler/spirv/vtn_private.h   | 14 ++++++++++++++
>>   src/compiler/spirv/vtn_variables.c | 14 ++++++++++++++
>>   2 files changed, 28 insertions(+)
>>
>> diff --git a/src/compiler/spirv/vtn_private.h b/src/compiler/spirv/vtn_private.h
>> index 59fcc94..a377c90 100644
>> --- a/src/compiler/spirv/vtn_private.h
>> +++ b/src/compiler/spirv/vtn_private.h
>> @@ -288,6 +288,20 @@ struct vtn_variable {
>>      nir_variable *var;
>>      nir_variable **members;
>>
>> +   /**
>> +    * In some early released versions of GLSLang, it implemented all function
>> +    * calls by making copies of all parameters into temporary variables and
>> +    * passing those variables into the function.  It even did so for samplers
>> +    * and images which violates the SPIR-V spec.  Unfortunately, two games
>> +    * (Talos Principle and Doom) shipped with this old version of GLSLang and
>> +    * also happen to pass samplers into functions.  Talos Principle received
>> +    * an update fairly shortly after release with an updated GLSLang.  Doom,
>> +    * on the other hand, has never received an update so we need to work
>> +    * around this GLSLang issue in SPIR-V -> NIR.  Hopefully, we can drop this
>> +    * hack at some point in the future.
>> +    */
>> +   struct vtn_access_chain *copy_prop_sampler;
>> +
>>      struct vtn_access_chain chain;
>>   };
>>
>> diff --git a/src/compiler/spirv/vtn_variables.c b/src/compiler/spirv/vtn_variables.c
>> index 0f0cc1c..974adb5 100644
>> --- a/src/compiler/spirv/vtn_variables.c
>> +++ b/src/compiler/spirv/vtn_variables.c
>> @@ -96,6 +96,10 @@ rewrite_deref_types(nir_deref *deref, const struct glsl_type *type)
>>   nir_deref_var *
>>   vtn_access_chain_to_deref(struct vtn_builder *b, struct vtn_access_chain *chain)
>>   {
>> +   /* Do on-the-fly copy propagation for samplers. */
>> +   if (chain->var->copy_prop_sampler)
>> +      return vtn_access_chain_to_deref(b, chain->var->copy_prop_sampler);
>> +
>>      nir_deref_var *deref_var;
>>      if (chain->var->var) {
>>         deref_var = nir_deref_var_create(b, chain->var->var);
>> @@ -1613,6 +1617,16 @@ vtn_handle_variables(struct vtn_builder *b, SpvOp opcode,
>>      case SpvOpStore: {
>>         struct vtn_access_chain *dest =
>>            vtn_value(b, w[1], vtn_value_type_access_chain)->access_chain;
>> +
>> +      if (glsl_type_is_sampler(dest->var->type->type)) {
>> +         vtn_warn("OpStore of a sampler detected.  Doing on-the-fly copy "
>> +                  "propagation to workaround the problem.");
>> +         assert(dest->var->copy_prop_sampler == NULL);
>> +         dest->var->copy_prop_sampler =
>> +            vtn_value(b, w[2], vtn_value_type_access_chain)->access_chain;
>> +         break;
>> +      }
>> +
>>         struct vtn_ssa_value *src = vtn_ssa_value(b, w[2]);
>>         vtn_variable_store(b, src, dest);
>>         break;
>> --
>> 2.5.0.400.gff86faf
>>
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>