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

[Jason Ekstrand]

On Tue, Jun 20, 2017 at 12:19 PM, Pierre-Loup A. Griffais <
pgriffais at valvesoftware.com> wrote:

> 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?
>

Well, it was a problem with Talos at the initial release but that's been
fixed.  The only that more recently crossed my radar was GfxBench 5.


>
> 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
>>>
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170620/3a8494da/attachment.html>